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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 6 inline comments as done.
sameerds added inline comments.


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


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1395-1398
+    if (!PotentialConstantsAA.isAtFixpoint()) {
+      Follow = false;
+      return;
+    }
----------------
jdoerfert wrote:
> sameerds wrote:
> > jdoerfert wrote:
> > > This doesn't make sense to me. We need to look at all VariableOffsets and decide. So `return` should only be present if we give up.
> > You might have missed the "not" in the condition. We want to give up and come again later if any information is not at fixed point. This is easily exercised by tests involving induction variables. The set of potential values of the induction variable keeps growing, but we should not use that set until it is fully enumerated. Any eager propagation of a non-fixed-point through PointerInfo at this point affects conclusions in other attributes that depend on it. I did not look for an example of correctness, but it does cause the attributor to retain stores that it would have otherwise removed in one existing test.
> We cannot wait in one AA for another to find a fixpoint. That is not sound. That is not even always possible. Even if it would be, it won't work in the current algorithm. You need to update the AA state based on the state of the other AA, always. Then signal if something changed.
> 
> That said, if we retain stores doing this properly we need to understand why.
You're right. What I did here was plain wrong. The root cause was that when me merge ranges in operator&= for Access objects, we conservatively drop the contents. We don't need to be so conservative ... just combining contents from the two Access objects works well in case both happen to have the same contents.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1405
+    }
+    NewOI = UnionOfAllCopies;
+  }
----------------
jdoerfert wrote:
> sameerds wrote:
> > jdoerfert wrote:
> > > I don;t follow why we need two extra OffsetInfo objects here. We modify NewOI anyway, no?
> > On each iteration of the outer for loop over VariableOffsets, the expression is a product:
> > 
> >   UnionOfAllCopies = NewOI x AssumedSet 
> > 
> > CopyPerOffset is the temporary used by the inner loop to compute this product. We need UnionOfAllCopies because it must only contain modified copies of NewOI, but not NewOI itself. We can't merge the RHS into NewOI. We have to start with an empty set.
> > 
> > The actual output of the function is UsrOI. We do not move NewOI into that if we exit early.
> > 
> > I realize there is no test for this yet. Working on that ... writing a GEP by hand is hard, when it involves nested aggregate types!
> > I realize there is no test for this yet. Working on that ... writing a GEP by hand is hard, when it involves nested aggregate types!
> 
> Use clang.
Test added.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1575
+        return false;
+      return true;
     }
----------------
jdoerfert wrote:
> Nit: Unsure why we need two returns here.
I missed this. If there are no other changes required, I can fix this locally before submitting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138646