[PATCH] D77528: [MLIR] Add support to use aligned_alloc to lower AllocOp from std to llvm

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added a comment.

It is my understanding this CL does not change previous behavior, is this accurate?



================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h:45
                                          OwningRewritePatternList &patterns,
+                                         bool alignedAlloc = false,
                                          bool emitCWrappers = false);
----------------
Same comment as in the parent revision, this is a silently breaking API change that will impact integrations.
I'd just use the struct that you'd have introduced in the previous revision and document that this is only using 2 fields.


================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h:66
 std::unique_ptr<OpPassBase<ModuleOp>> createLowerToLLVMPass(
-    bool useBarePtrCallConv = false, bool emitCWrappers = false,
+    bool alignedAlloc = false, bool useBarePtrCallConv = false,
+    bool emitCWrappers = false,
----------------
same discussion re silently breaking API changes


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1427
+      uint64_t constEltSizeBytes = 0;
+      auto isMallocAlignmentSufficient = [&]() {
+        if (auto vectorType = elementType.template dyn_cast<VectorType>())
----------------
bondhugula wrote:
> ftynse wrote:
> > bondhugula wrote:
> > > ftynse wrote:
> > > > I wouldn't add a lambda that is only called once immediately after its definition.
> > > Hmm... this is just for better readability - it gives a name / auto documents a code block without the need to outline it into a function or add an explicit comment. I've seen this as a standard practice.
> > This does not seem to be common practice in MLIR. FWIW, I find it less readable than just writing
> > 
> > ```
> > int64_t constEltSizeBytes = 0;
> > if (auto vectorType = elementType.template dyn_cast<VectorType>())
> >   constEltSizeBytes =
> >       vectorType.getNumElements() *
> >       llvm::divideCeil(vectorType.getElementTypeBitWidth(), 8);
> > else
> >   constEltSizeBytes =
> >       llvm::divideCeil(elementType.getIntOrFloatBitWidth(), 8);
> > 
> > // Use aligned_alloc if elt_size > malloc's alignment.
> > bool isMallocAlignmentSufficient = constEltSizeBytes > kMallocAlignment;
> > useAlignedAlloc = isMallocAlignmentSufficient;
> > ```
> > 
> > Since you already have the block comment immediately above it anyway, and variables can have names just as well as lambdas. The lambda also mutates a global state that it captures by-reference, so the only effects of lambda are: (1) extra indentation; (2) extra LoC; and (3) extra concepts leading to cognitive overhead.
> Hmm... the demarcation/isolation is important I feel. I'm fine with changing to the straightline style but out of curiosity and for future purposes as well, it'll be good to have a third person view here on coding style as far as such patterns go: @mehdi_amini - is there a guideline here?
I am generally a fan of such style (esp. when mixed with functional combinators), so I'd vote for +1 it when it makes sense.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1408
+    // Whenever we don't have alignment set, we will still use aligned_alloc
+    // if the alignment needed is more than what malloc can provide.
+    uint64_t constEltSizeBytes = 0;
----------------
Alignment needed is really a target-specific thing isn't it?
I would have expected something that looks at DataLayout and that brings in the can of worms we have been punting on (I understand once flang is in MLIR core we will want to reopen it).

Can this part be dropped from this revision, esp in light of @ftynse's comments?

I also have some fun micro ARM targets I will need to test some of this on, the smaller the baked in assumptions the better.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77528