[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 2 inline comments as done.
bondhugula added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1589
+  // This is the alignment malloc typically provides.
+  constexpr static unsigned kMallocAlignment = 16;
 };
----------------
ftynse wrote:
> bondhugula wrote:
> > ftynse wrote:
> > > bondhugula wrote:
> > > > ftynse wrote:
> > > > > I am a bit concerned about hardcoding the "typical" value. Can we make it parametric instead?
> > > > I had sort of a similar concern. But 16 bytes is pretty much what glibc malloc gives on nearly every system we have (on probably really old ones, it was perhaps 8 bytes). Did you want a pass flag and then letting 16 be the default - that would be too much plumbing (just like alignedAlloc). This is already a parameter of sorts. 
> > > The world is not limited to glibc. MLIR should also work on other platforms, and you essentially shift the burden of the plumbing you didn't do to people debugging builds on those platforms. You can have one pass option that corresponds to malloc alignment and, if it is set to 0, treat it as "never use aligned_alloc".
> > Sorry, I didn't quite understand. What should the pass options be and what should the behavior and the default behavior be?
> Normally, you would have two pass options (and a configuration `struct` for the constructors like Nicolas proposed in another patch to decrease the amount of churn in pass constructor APIs): `-use-aligned-alloc` and `-assume-malloc-alignment`. If you don't want two separate options, you could get away with one `-use-aligned-alloc-and-assume-malloc-alignment` (did not think about a better name). If it is set to zero (default), the conversion doesn't use aligned_alloc at all. If it is set to non-zero, the conversion uses aligned_alloc and treats the option value as malloc alignment in order to also use aligned_alloc in relevant cases.
The configuration struct is now done (in the parent revision). How about just keeping it simple with -use-aligned-alloc and not changing previous/existing behavior when -use-aligned-alloc is not provided? This revision is not about tinkering with malloc alignment handling. 

Update - PTAL. Thanks for all the feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77528