[PATCH] D75329: [MLIR] Added llvm.freeze

Fri Feb 28 01:21:30 PST 2020

ftynse added a comment.

Looks good, I mostly have nits.

In D75329#1897313 <https://reviews.llvm.org/D75329#1897313>, @echristo wrote:

> Can you document the semantics somewhere? (I have no idea if other patches have done anything like this, but it does seem like something that should be documented somewhere).

In general, we should just refer to the relevant piece of LLVM's LangRef unless MLIR modeling has different semantics than LLVM IR (which we should avoid as much as possible).

================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:511
     // FIXME: landingpad
-};
+    INST(Freeze, Freeze)};
 #undef INST
----------------
Nit: could you keep these ordered in the same way as LLVM's LangRef?

================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:288
+func @useFreezeOp(%arg0: !llvm.i32) {
+  // CHECK:  = llvm.freeze %arg0 : !llvm.i32
+  %0 = llvm.freeze %arg0 : !llvm.i32
----------------
Please avoid matching SSA names (`%arg0`). You can capture by putting `// CHECK-SAME: %[[ARG0:.*]]:` after the label

================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:290
+  %0 = llvm.freeze %arg0 : !llvm.i32
+  // CHECK: %[[x:[0-9]+]] = llvm.mlir.undef : !llvm.i8
+  %1 = llvm.mlir.undef : !llvm.i8
----------------
Nit: we usually just do `%[[x:.*]]`

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75329/new/

https://reviews.llvm.org/D75329