[PATCH] D113888: [SDAG] Use UnknownSize for masked load/store MMO size

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

> Does this mean any operation that's a mayload/maystore needs to have the size of the operand set to "unknown"?  I suspect we need to do significantly more work than just this patch if we want memory operands to work this way.  (e.g. type legalization, if conversion.)  What happens if we just say that size of a memory location is supposed to represent an upper bound, at least for now?

Why would it be any operation, not just masked loads/stores? The problem is in this case the underlying object only had 10 dereferenceable chars, where as the entire vector width is 16bytes. I don't think that would apply to any normal load/store - it would already be UB at the llvm-ir level.

It's the equivalent of this code, but there is unfortunately no current way of adding MemoryLocations directly, it goes via a LLT size. Maybe we do want to change that, but I didn't want to try and do it here. It looks quite involved.
https://github.com/llvm/llvm-project/blob/913d78c40c37c9c3428285d868ce454b058e40f3/llvm/lib/Analysis/MemoryLocation.cpp#L165

The exact thing going wrong here is that this code in the scheduler is adding chain dependencies:
https://github.com/llvm/llvm-project/blob/913d78c40c37c9c3428285d868ce454b058e40f3/llvm/lib/CodeGen/ScheduleDAGInstrs.cpp#L942
Which checks this AA check, which returns false, I think from basic-aa:
https://github.com/llvm/llvm-project/blob/913d78c40c37c9c3428285d868ce454b058e40f3/llvm/lib/CodeGen/ScheduleDAGInstrs.cpp#L543
So no scheduling dependencies between the instructions and we end up in the wrong order. This started happening after we moved the VPT block pass later so predicated MVE instructions are no long in bundles during the post-ra scheduler.

>> we shouldn't be passing MemoryLocation::UnknownSize
>
> If that's true, maybe we need to assert this?  Apparently we are actually passing in MemoryLocation::UnknownSize in some places.

Sure that sounds good, I can take a look. It might be simpler to make sute the relevant methods work sensibly with MemoryLocation::UnknownSize.


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

https://reviews.llvm.org/D113888