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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1404
+  static Optional<int64_t> getAllocationAlignment(Operation *op,
+                                                  bool &useAlignedAlloc) {
+    auto elementType = cast<AllocLikeOp>(op).getType().getElementType();
----------------
bondhugula wrote:
> ftynse wrote:
> > bondhugula wrote:
> > > ftynse wrote:
> > > > Can't you just rely on the Optional result being None to signal that aligned alloc is not required?
> > > We still use aligned_alloc if the elt size is > 16 bytes (in spite of no alignment attribute). The code would otherwise crash since the loads/stores won't have alignment guaranteed (with malloc) but the they would be lowered by LLVM via aligned load/stores. (LLVM in the absence of alignment attributes on load/store would assume ABI alignment which for say vector<8xf32> elt types would be 256-bit boundaries). 
> > > We still use aligned_alloc if the elt size is > 16 bytes
> > 
> > At which point, this function returns the alignment value rather than `llvm::None`.
> > The only situation where it returns a non-`None` value and sets `useAlignedAlloc` to `false` is for `AllocaOp`. I would consider refactoring this function to only work on `AllocOp`, and querying the `AllocaOp` allocation separately in the call place. This would make the API simpler and shorten the code. And you wouldn't need an extra boolean result.
> > 
> > > (LLVM in the absence of alignment attributes on load/store would assume ABI alignment which for say vector<8xf32> elt types would be 256-bit boundaries).
> > 
> > I suppose we can just take the ABI alignment into account when lowering the `alloc` operation (we already do so for the `alignment` attribute). Not all platforms have `aligned_alloc`.
> >I would consider refactoring this function to only work on 
> >AllocOp, and querying the AllocaOp allocation separately in the 
> 
> Sure, sounds good.
> 
> >I suppose we can just take the ABI alignment into account when 
> >lowering the alloc operation (we already do so for the
> 
> Yes, we should do this with malloc as well. This can be a TODO and should be done in a separate revision (because it isn't to do with aligned_alloc but fixing malloc lowering).
> Not all platforms have aligned_alloc.

Btw, aligned_alloc is a C11 and C++11 standard, and it's available on Windows too (it's posix_memalign that isn't). And of course there are systems where there isn't any dynamic memory allocation at all. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77528