[PATCH] D76602: [MLIR] Introduce std.alloca op

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache accepted this revision.
nicolasvasilache added a comment.
This revision is now accepted and ready to land.

Thanks Uday, looks good!

Accepting conditioned on the LLVM options struct change.



================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:144
+    "Builder *builder, OperationState &result, MemRefType memrefType, " #
+    "ValueRange operands, IntegerAttr alignment = IntegerAttr()", [{
+       result.addOperands(operands);
----------------
Note: I tend to prefer extra builders that take all PODs instead of attributes.
They are more natural to use and just work nicely once wrapped into EDSCs.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:304
+    %0 = alloc()[%s] : memref<8x64xf32,
+                              affine_map<(d0, d1)[s0] -> ((d0 + s0), %d1)>, 1>
     ```
----------------
typo %d1


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1293
       // offset = (align - (ptr % align))% align
       Value intVal = rewriter.create<LLVM::PtrToIntOp>(
+          loc, this->getIndexType(), allocatedBytePtr);
----------------
I think the change in indentation (?) here and everywhere is unfortunate, it does not properly track what code moved (materialized with a yellow phab horizontal bar on the left) and make it harder to see what changed vs what moved.

I am mostly eyeballing the code after realizing most of this is just moved.

Please flag specific things need deeper review if appropriate. 


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:2905
 std::unique_ptr<OpPassBase<ModuleOp>>
-mlir::createLowerToLLVMPass(bool useAlloca, bool useBarePtrCallConv,
-                            bool emitCWrappers, unsigned indexBitwidth) {
-  return std::make_unique<LLVMLoweringPass>(useAlloca, useBarePtrCallConv,
-                                            emitCWrappers, indexBitwidth);
+mlir::createLowerToLLVMPass(bool useBarePtrCallConv, bool emitCWrappers,
+                            unsigned indexBitwidth) {
----------------
I am concerned by the silent behavior breaking changes here and in the followup revision.
I am expecting this will be painful for integrations and will repeat itself.

Can we turn this into a:
```
struct LowerToLLVMOptions {
  ... 
} 
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76602