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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added inline comments.


================
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);
----------------
nicolasvasilache wrote:
> 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.
Hmm... I didn't change the existing alloc op builder here; just using the same for Alloca op. Yes, this can be changed to just take int64_t in another revision. 


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1293
       // offset = (align - (ptr % align))% align
       Value intVal = rewriter.create<LLVM::PtrToIntOp>(
+          loc, this->getIndexType(), allocatedBytePtr);
----------------
nicolasvasilache wrote:
> 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. 
All of this is just moved. Nothing in the alloc op lowering actually changed with this - I'm just reusing the common parts for alloca. 


================
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) {
----------------
nicolasvasilache wrote:
> 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 {
>   ... 
> } 
> ```
> 
> 
That's a concern - there are going to be silent breaking effects.  I've changed it to struct here. But even with a struct, you'll have the same issue with list initialization (some of the fields will remain uninitialized / wrongly initialized depending on where the field was removed from or where the new field was added). If the new fields are always added at the end, we'll just have uninitialized fields. And with explicit field-wise init, it'll just lead to uninitialized stuff.

BTW, none of the three options are documented at the declaration (but only in Passes.td).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76602