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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert 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:
> > 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].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138646