[PATCH] D138646: [AAPointerInfo] track multiple constant offsets for each use

Sameer Sahasrabuddhe via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 7 22:43:36 PST 2022


sameerds marked an inline comment as not done.
sameerds added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5209
+      // present in the union, then any MUST needs to be removed.
       Kind = AccessKind(Kind | R.Kind);
+      if ((Kind & AK_MAY) || Ranges.size() > 1) {
----------------
sameerds wrote:
> jdoerfert wrote:
> > sameerds wrote:
> > > jdoerfert wrote:
> > > > sameerds wrote:
> > > > > jdoerfert wrote:
> > > > > > It was dropping the results for a reason before, just going back on it is probably not sound either.
> > > > > > 
> > > > > > If we have:
> > > > > > ```
> > > > > > range: [0-4]
> > > > > > value: i32 -42
> > > > > > ```
> > > > > > and merge it with
> > > > > > ```
> > > > > > range: [0-8]
> > > > > > value: i64 -42
> > > > > > ```
> > > > > > we need to do something here.
> > > > > > At least, we need to change must to may but I think we cannot even keep the value if they are equal under under zext.
> > > > > > If we would, writing `i32 0` and `i64 0` would make us believe the former writes 8 bytes and they'll all be 0, which is not true.
> > > > > > 
> > > > > > When does this happen, maybe we need to understand the problem better.
> > > > > The Access objects being merged here originate from the same remote instruction. Doesn't that mean that the type is guaranteed to be the same? I tried putting an assert(Ty == R.Ty), and it did not fire for the lit tests. Instead of asserting, we can always check that before calling combineInValueLattice().
> > > > I'm still confused. Even if it's the same value this is a problem, no?
> > > > ```
> > > > range: [0-4]
> > > > value: i32 4
> > > > must write
> > > > ```
> > > > merged with
> > > > ```
> > > > range: [4-8]
> > > > value: i32 4
> > > > must write
> > > > ```
> > > > will result in
> > > > ```
> > > > range: [0-8]
> > > > value: i32 4
> > > > must write
> > > > ```
> > > > which is not true. For one, we may only write 4 out of 8 bytes, and depending which ones its not going to be `4` if you read the range [0-8].
> > > Here is what I see about the creation of Access objects:
> > > 
> > >   - Each Access is for a unique value.
> > >   - If the value is a MemInstrinsic, then the length is known, and all ranges for that Access will have that same length.
> > >   - Else, if the value is an argument, then the length is unknown and the Access has only one range (unknown).
> > >   - Else, the value is an instruction with an optionally known type (see handleAccess()). If the length is known, then all ranges have the same length, else it is a single unknown range.
> > > 
> > > This invariant is maintained even when looking through function calls.
> > > 
> > > If the above is correct, then it might be redundant to even track the size for every Range in a RangeList. But that is assuming Ranges are used only for PointerInfo::Access objects. If the Range should remain generic, then we should allow the possibility that all Ranges in a RangeList are not the same size. We could add a bool "AllRangesAreSameSize" and check this when merging an Access into another Access.
> > > 
> > > So if Ranges are not the same size, then the contents are unknown. Else, if the types are the same, then combine the contents. Else the contents are unknown.
> > I'm very confused. Apologies.
> > 
> > So, if I understand this correctly now we will keep the value but mark the access as MAY if it has more than one range. That seems correct. My example above was merging the ranges, which we are not, and also keeping the MUST bit, which we are not. So, assuming I finally understand what is happening this should be fine.
> Correction in the third bullet ... the length **may** be unknown if it is not llvm::Argument. Otherwise the invariant about looking through function calls applies.
I am just glad to have your attention while I work through this! It's an important optimization for launching HIP programs.

So yes, if there are multiple ranges in an Access, they are of the same size. We keep the contents if possible, but it is always a MAY access. I have put asserts in strategic places (isMayAccess and isMustAccess) to catch that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138646



More information about the llvm-commits mailing list