Added LLVM ops and lowering phases from standard dialect. #300

dfki-jugr · 2019-12-06T14:15:55Z

Added LLVM ops and lowering phases from standard dialect for FAbs, FCeil, Cos, FNeg, CopySign and Tanh.

Added test cases for the newly added LLVM operations and lowering features.

pifon2a · 2019-12-06T14:22:34Z

lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp

@@ -793,12 +790,33 @@ struct SubFOpLowering : public BinaryOpLLVMOpLowering<SubFOp, LLVM::FSubOp> {
 struct MulFOpLowering : public BinaryOpLLVMOpLowering<MulFOp, LLVM::FMulOp> {
  using Super::Super;
 };


Maybe have unary ops defs before binary ops sorted and also sort them?

test/Target/llvmir-intrinsics.mlir

pifon2a · 2019-12-06T14:26:37Z

include/mlir/Dialect/LLVMIR/LLVMOps.td

+  }];
+}
+
+def LLVM_TanhOp : LLVM_Op<"tanh", [NoSideEffect]>,


@River707, @ftynse, @joker-eph
What would be the right way to test values of Tanh? It is quite easy to introduce a bug.

We need a more detailed explanation of where these values come from and what is computed in the end.

pifon2a · 2019-12-06T14:31:31Z

include/mlir/Dialect/LLVMIR/LLVMOps.td

+  }];
+}
+
+def LLVM_CosOp : LLVM_Op<"intr.cos", [NoSideEffect]>,


nit: sort defs (CosOp should be after Copysign)

ftynse

LLVM dialect should only contain LLVM instructions or intrinsics. There's no tanh in LLVM IR, so it should not be in the dialect. Consider implementing what you did in the builder as a conversion pattern from tanh to a combination of LLVM dialect operations.

ftynse · 2019-12-06T14:22:48Z

include/mlir/Dialect/LLVMIR/LLVMOps.td

+                   Arguments<(ins LLVM_Type:$in)>,
+                   Results<(outs LLVM_Type:$res)> {
+  let llvmBuilder = [{
+    llvm::Module *module = builder.GetInsertBlock()->getModule();


As we keep adding more intrinsics, I am more and more interesting in generalizing these. Have you considered it?

ftynse · 2019-12-06T14:25:38Z

include/mlir/Dialect/LLVMIR/LLVMOps.td

+  }];
+}
+
+def LLVM_FCeilOp : LLVM_Op<"intr.ceil", [NoSideEffect]>,


There's a mismatch between op name (fceil) and intrinsic name (ceil), please fix.

ftynse · 2019-12-06T14:26:36Z

include/mlir/Dialect/LLVMIR/LLVMOps.td

+def LLVM_TanhOp : LLVM_Op<"tanh", [NoSideEffect]>,
+                   Arguments<(ins LLVM_Type:$in)>,
+                   Results<(outs LLVM_Type:$res)> {
+  let llvmBuilder = [{


This is way too long to be inline code in Tablegen. Please extract into a function and just call it here.

@dfki-jugr Let's remove Tanh from the current PR. This logic be added to legalization from STD TanhOp to LLVM dialect, not from LLVM dialect to LLVM IR.

ftynse · 2019-12-06T14:27:40Z

include/mlir/Dialect/LLVMIR/LLVMOps.td

+    llvm::Module *module = builder.GetInsertBlock()->getModule();
+
+    llvm::Type *type = $in->getType();
+    llvm::Value *max_value = llvm::ConstantFP::get(type, 20.0);


MLIR uses camelBack for variable names (see https://github.com/tensorflow/mlir/blob/master/g3doc/DeveloperGuide.md).

ftynse · 2019-12-06T14:28:15Z

include/mlir/Dialect/LLVMIR/LLVMOps.td

+    llvm::Value *input_clamped =
+      builder.CreateSelect(cmp_lower_bound, clamp_min, select_res);
+
+    static constexpr std::array<float, 7> numerator_coeffs{


The must be accompanied by a detailed comment of what you are doing here and why it should work.

ftynse · 2019-12-06T15:18:26Z

include/mlir/Dialect/LLVMIR/LLVMOps.td

+  }];
+}
+
+def LLVM_TanhOp : LLVM_Op<"tanh", [NoSideEffect]>,


If there is no tanh op in LLVM, it must not be an LLVM Op. Create it in some dialect and implement a lowering from that dialect to the LLVM dialect instead, so the translation from the LLVM dialect to LLVM IR is as simple as possible. Having magic constants in the translation wouldn't cut it.

If you implement your non-trivial conversion in as a conversion pattern, it is significantly easier to replace.

ftynse · 2019-12-06T15:23:32Z

please do not merge this until the concerns on translation are addressed.

pifon2a

We can submit this CL, after the TanhOp lowering is excluded and the comments are addressed.

test/Target/llvmir-intrinsics.mlir

pifon2a · 2019-12-12T14:25:39Z

include/mlir/Dialect/LLVMIR/LLVMOps.td

+def LLVM_CosOp : LLVM_UnaryIntrinsicOp<"cos">;
+
+def LLVM_CopySignOp : LLVM_BinaryIntrinsicOp<"copysign">;
+


nit: remove empty line

…eil, Cos, FNeg, CopySign and Tanh. Added test cases for the newly added LLVM operations and lowering features.

…eil, Cos, FNeg, CopySign. Added test cases for the newly added LLVM operations and lowering features. Closes #300 COPYBARA_INTEGRATE_REVIEW=tensorflow/mlir#300 from dfki-jugr:std_to_llvm da6168bbc1a369ae2e99ad3881fdddd82f075dd4 PiperOrigin-RevId: 286231169 Change-Id: I364bda2adad4054052a3a53b4cb1c3de1102864f

…eil, Cos, FNeg, CopySign. Added test cases for the newly added LLVM operations and lowering features. Closes tensorflow/mlir#300 COPYBARA_INTEGRATE_REVIEW=tensorflow/mlir#300 from dfki-jugr:std_to_llvm da6168bbc1a369ae2e99ad3881fdddd82f075dd4 PiperOrigin-RevId: 286231169

googlebot added the cla: yes label Dec 6, 2019

pifon2a self-requested a review December 6, 2019 14:17

pifon2a approved these changes Dec 6, 2019

View reviewed changes

ftynse suggested changes Dec 6, 2019

View reviewed changes

pifon2a suggested changes Dec 6, 2019

View reviewed changes

pifon2a suggested changes Dec 12, 2019

View reviewed changes

Added LLVM ops and lowering phases from standard dialect for FAbs, FC…

da6168b

…eil, Cos, FNeg, CopySign and Tanh. Added test cases for the newly added LLVM operations and lowering features.

dfki-jugr force-pushed the std_to_llvm branch from 7792dd7 to da6168b Compare December 13, 2019 11:27

pifon2a added the ready to pull label Dec 13, 2019

pifon2a approved these changes Dec 13, 2019

View reviewed changes

pifon2a added ready to pull and removed ready to pull labels Dec 13, 2019

pifon2a approved these changes Dec 16, 2019

View reviewed changes

ftynse approved these changes Dec 17, 2019

View reviewed changes

ftynse added the kokoro:run label Dec 17, 2019

kokoro-team removed the kokoro:run label Dec 17, 2019

mlir-copybara-bot closed this in 22781f6 Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added LLVM ops and lowering phases from standard dialect. #300

Added LLVM ops and lowering phases from standard dialect. #300

dfki-jugr commented Dec 6, 2019

pifon2a Dec 6, 2019

pifon2a Dec 6, 2019

ftynse Dec 6, 2019

pifon2a Dec 6, 2019

ftynse left a comment

ftynse Dec 6, 2019

ftynse Dec 6, 2019

ftynse Dec 6, 2019

pifon2a Dec 6, 2019

ftynse Dec 6, 2019

ftynse Dec 6, 2019

ftynse Dec 6, 2019 •

edited

Loading

ftynse commented Dec 6, 2019

pifon2a left a comment

pifon2a Dec 12, 2019

		def LLVM_CosOp : LLVM_UnaryIntrinsicOp<"cos">;

		def LLVM_CopySignOp : LLVM_BinaryIntrinsicOp<"copysign">;

Added LLVM ops and lowering phases from standard dialect. #300

Added LLVM ops and lowering phases from standard dialect. #300

Conversation

dfki-jugr commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftynse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftynse Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

ftynse commented Dec 6, 2019

pifon2a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftynse Dec 6, 2019 •

edited

Loading