[PATCH] D79230: [MLIR][crashfix] Fall back to malloc when alignment is 0 for align-alloc

Alex Zinenko via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 4 02:38:00 PDT 2020


ftynse added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1524
     // Whether to use std lib function aligned_alloc that supports alignment.
-    bool useAlignedAlloc = allocationAlignment.hasValue();
+    bool useAlignedAlloc = allocationAlignment && *allocationAlignment != 0;
 
----------------
kernhanda wrote:
> bondhugula wrote:
> > The alignment is actually required to be a power of two for aligned_alloc. The code above was written with the assumption that the alignment would be a power of two. Unfortunately, we don't document what a valid `alignment` on the AllocOp is. Unlike llvm's alloca where a 0 alignment is treated the same as not specifying an alignment, we could make the alloc op's verifier fail for non power of two alignments. This will fix this case as well - since zero is not a power of two. 
> > 
> > This fix will silently fall back to malloc for '0' alignment instead of trying to use aligned_alloc with elt size alignment (which is what happens if no alignment is set). If we wanted to handle this case, the right fix would be to check for a non-power of two alignment (in `getAllocationAlignment`), and use the default elt size based alignment while still using aligned_alloc. 
> I'm okay with either of the alternatives, if others have an opinion.
I agree with @bondhugula that it should be fixed at the op verifier level. Arguably, silently falling back to the strategy that was explicitly disabled by an option (i.e. using malloc instead of aligned_alloc) is a surprising and undesirable behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79230





More information about the llvm-commits mailing list