[PATCH] D148930: [MemRefToLLVM] Fix the lowering of memref.assume_alignment

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet marked an inline comment as done.
qcolombet added inline comments.


================
Comment at: mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp:372
     Value ptrValue = rewriter.create<LLVM::PtrToIntOp>(loc, intPtrType, ptr);
     rewriter.create<LLVM::AssumeOp>(
         loc, rewriter.create<LLVM::ICmpOp>(
----------------
mravishankar wrote:
> qcolombet wrote:
> > I was talking with @mravishankar offline and he pointed out that if we are aligned at (alignPtr + offset) then alignPtr must be aligned at least with the same alignment.
> > I.e., we could issue two assumes here:
> > - one for the aligned base pointer.
> > - one for the aligned base pointer + offset.
> > 
> > It sounded like useful information to lower.
> > 
> > What do you guys think?
> To clarify this a bit. If you need an `basePtr + offset` to be aligned to a value v, you need both `basePtr` and `offset` to be aligned to `v`. Without that you cannot guarantee that `basePtr + offset` is always aligned to `v` for all values of `basePtr`. 
> This is the reverse logic though. Here it is saying `basePtr + offset` is aligned, but I am not sure if can always assume that `basePtr` is aligned. Maybe we skip it for now... In most common case though, `basePtr` is also aligned. We can let the user (in this case IREE) generate `assume` for both.
Thinking about it a little more, I think it is better to let the producers of `memref.assume_alignment` to separately generate one for the base pointer if they want.
I can think of counter examples where this wouldn't be correct to issue that assume.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148930