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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 2 inline comments as 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) {
----------------
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().


================
Comment at: llvm/test/Transforms/Attributor/multiple-offsets-pointer-info.ll:336
+  ret i8 %i
+}
+
----------------
jdoerfert wrote:
> 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.
Exactly what I had in mind. Would prefer to do this as a follow-up. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138646