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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1427
+      uint64_t constEltSizeBytes = 0;
+      auto isMallocAlignmentSufficient = [&]() {
+        if (auto vectorType = elementType.template dyn_cast<VectorType>())
----------------
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 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77528