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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked an inline comment as done.
bondhugula added a comment.

@nicolasvasilache wrote:

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

It actually changes it in one case: if the alloc op didn't have alignment specified *and* its elt type was larger than 16 bytes, this revision would make it use aligned_alloc. I can change this and let it continue to use malloc, but note that the malloc in such cases would have generated code that would crash. So this is only fixing things for the previous behavior it's changing. And we should also change malloc path to do the alignment to elt type boundaries for > 16 bytes by default.



================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1427
+      uint64_t constEltSizeBytes = 0;
+      auto isMallocAlignmentSufficient = [&]() {
+        if (auto vectorType = elementType.template dyn_cast<VectorType>())
----------------
mehdi_amini wrote:
> ftynse wrote:
> > nicolasvasilache wrote:
> > > 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.
> > Generally, I would be strongly opposed to defining style guidelines based on a single use of a construct in a single diff, where only a small subset of contributors could participate (and before you can object that review history is public, are you reading all comments on all diffs?). I would be also opposed to having to define additional rules until we have to. I am not generally opposed to helper lambdas, I just don't see any benefit from this specific one, only drawbacks. And a lambda that the entire environment by reference is not exactly my definition of isolation.
> @bondhugula this seems too detailed to have a guideline :)
> I wouldn't say it is "common", but probably not unheard of?
> I have been doing this myself but in general not calling it right after, rather to outline a block of code outside of a loop to make the loop shorter and easier to read, or similar situation (getting large boilerplate out of the way and "naming it").
> 
> @rriddle ?
As Mehdi now confirms, this is too detailed to have a style guideline. The lambda demarcates the start and end of the thing it's naming/auto-documenting - you don't get it from code comments alone and I see it better for readability. Given @ntv's and @mehdi_amini's comments, I'm now strongly inclined to retain it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77528