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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

The only "issue" I have with this is the select traversal. We should rely on AAPotentialValues here, see the comment below. Everything else makes sense and looks pretty good.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5142
+    const RangeTy &getUnique() const {
+      assert(isUnique());
+      return Ranges.front();
----------------
All assertions need messages please, also elsewhere.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1497
+        UsrOI.setUnknown();
+      }
+
----------------
I think we might want to call `Attributor::getPotentialValues` on the variable offsets. It should handle select and phi and more, e.g., loads.
Maybe in addition to `Attributor::getAssumedConstant` we should have `Attributor::getAssumedConstantValues`.
I want to avoid yet another traversal of some instructions in favor of common interfaces.a


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1543
+        auto BaseOI = It->getSecond();
+        BaseOI.addToAll(Offset.getZExtValue());
+        if (IsFirstPHIUser || BaseOI == UsrOI) {
----------------
SExt, I think. -1 is a fine offset.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1575
+        return false;
+      return true;
     }
----------------
Nit: Unsure why we need two returns here.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1604
+        return false;
+      return true;
     };
----------------
Same as above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138646