[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) {
----------------
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.


================
Comment at: llvm/test/Transforms/Attributor/multiple-offsets-pointer-info.ll:336
+  ret i8 %i
+}
+
----------------
AAPotentialConstantValues uses AAPotentialValues but the case to handle PHINodes is missing here: https://github.com/llvm/llvm-project/blob/72d76a2403459a38a1d6daae62de6945097db8f9/llvm/lib/Transforms/IPO/AttributorAttributes.cpp#L9222
We just need to go over the operands and call the fillSetWithConstantValues function. We can do that in a follow up or pre-patch, just commenting here to make sure we don't forget.


================
Comment at: llvm/test/Transforms/Attributor/value-simplify-pointer-info.ll:3154
+  %gep.loaded = getelementptr inbounds [1024 x i8], [1024 x i8]* %Bytes, i64 0, i64 %offset
+  store i8 100, i8* %gep.loaded, align 4
+
----------------
I think this is the same problem, we do not handle LoadInst and should just call fillSetWithConstantValues on the load value itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138646