[Attributor] Reach optimistic fixpoint in AAValueSimplify when the value is constant or undef

[uenoku]

Summary

As discussed in D71799, we have found that it is more useful to reach an optimistic fixpoint in AAValueSimpify when the value is constant or undef.

[baziotis]

I'm not super familiar with AAValueSimplify, but here's a perspective: The conceptual idea is that AAValueSimplify returns optimistic fixpoint if it actually changed the value.
We may not want to break that. Instead, keep the "contract" to whoever is using it to: Use that assuming that if you got a known value, then it means it changed from the original.
The implication being: Check for yourself if it is something that won't change (i.e. it is constant). One one hand that puts more logistics to the user. On the other hand, the code may become more
understandable as the check for constant (or undef) happens with regular LLVM values that anyone who is not familiar with the Attributor can understand what is happening (i.e. from looking the code, the reader knows that if the value is constant, it won't even go through AAValueSimplify).
Edit: Well, AFAIK obviously the user of AAValueSimplify will be another part of the Attributor, so the reader is supposed to be familiar with it. But still, I think the fact that we'll go through AAValueSimplify only if we have some kind of complicated value is makes the code more understandable.

[jdoerfert]

LGTM

[jdoerfert]

In D71852#1795513, @baziotis wrote:

I'm not super familiar with AAValueSimplify, but here's a perspective: The conceptual idea is that AAValueSimplify returns optimistic fixpoint if it actually changed the value.
We may not want to break that. Instead, keep the "contract" to whoever is using it to: Use that assuming that if you got a known value, then it means it changed from the original.
The implication being: Check for yourself if it is something that won't change (i.e. it is constant). One one hand that puts more logistics to the user. On the other hand, the code may become more
understandable as the check for constant (or undef) happens with regular LLVM values that anyone who is not familiar with the Attributor can understand what is happening (i.e. from looking the code, the reader knows that if the value is constant, it won't even go through AAValueSimplify).
Edit: Well, AFAIK obviously the user of AAValueSimplify will be another part of the Attributor, so the reader is supposed to be familiar with it. But still, I think the fact that we'll go through AAValueSimplify only if we have some kind of complicated value is makes the code more understandable.

Sorry, I didn't see your comment at first.

AAValueSimplify computes the best simplified value, it is considered "assumed" as long as we cannot prove it is correct, once that happens it is "known". In case of a constant (or undef which should actually be a constant), there is not much to do as it is the most simplified version (in almost all cases), thus we know it is a correct value.

@uenoku Thinking about it again, we could also not set the "invalid" bit when we call indicatePessimisticFixpoint and instead just set SimplifiedAssociatedValue to the associated one and make it an optimistic fixpoint.

llvm/lib/Transforms/IPO/Attributor.cpp

4081

Nit: I think Undef is a constant making the second part of the conditional obsolete.

[baziotis]

In D71852#1795715, @jdoerfert wrote:

Sorry, I didn't see your comment at first.

No problem.

AAValueSimplify computes the best simplified value, it is considered "assumed" as long as we cannot prove it is correct, once that happens it is "known". In case of a constant (or undef which should actually be a constant), there is not much to do as it is the most simplified version (in almost all cases), thus we know it is a correct value.

Aha yes. Note that this was my initial interpretation as well, hence why I did not understand why was happening what was happening before this change (e.g. at the end of https://reviews.llvm.org/D71799#1795477).
But then, @uenoku's explanation regarding that "simplified" implies "changed" made me think of a different perspective. All in all, since we agree on a common statement of what AAValueSimplified does, everything's ok. :)

[uenoku]

Address comment.

[uenoku]

In D71852#1795717, @baziotis wrote:

In D71852#1795715, @jdoerfert wrote:

Sorry, I didn't see your comment at first.

No problem.

AAValueSimplify computes the best simplified value, it is considered "assumed" as long as we cannot prove it is correct, once that happens it is "known". In case of a constant (or undef which should actually be a constant), there is not much to do as it is the most simplified version (in almost all cases), thus we know it is a correct value.

Aha yes. Note that this was my initial interpretation as well, hence why I did not understand why was happening what was happening before this change (e.g. at the end of https://reviews.llvm.org/D71799#1795477).
But then, @uenoku's explanation regarding that "simplified" implies "changed" made me think of a different perspective. All in all, since we agree on a common statement of what AAValueSimplified does, everything's ok. :)

In the beginning, I didn't think it was useful to regard constant as a known simplified value because you can trivially check the associated value. But anyway I agree that regarding constant as "simplified" will be more natural and concise.

[uenoku]

Small fix.

llvm/lib/Transforms/IPO/Attributor.cpp

if (auto *C = dyn_cast<Constant>(SimplifiedAssociatedValue.getValue())) {
         replaceAllInstructionUsesWith(V, *C);
         Changed = ChangeStatus::CHANGED;
       }
     }
 
     return Changed | AAValueSimplify::manifest(A);
   }
 
+  /// See AbstractState::indicatePessimisticFixpoint(...).
+  ChangeStatus indicatePessimisticFixpoint() override {
+    // NOTE: Associated value will be returned in a pessimistic fixpoint and is
+    // regarded as known. That's why`indicateOptimisticFixpoint` is called.
+    SimplifiedAssociatedValue = &getAssociatedValue();
+    return indicateOptimisticFixpoint();
+  }
+
 protected:
   // An assumed simplified value. Initially, it is set to Optional::None, which
   // means that the value is not clear under current assumption. If in the
   // pessimistic state, getAssumedSimplifiedValue doesn't return this value but
   // returns orignal associated value.
   Optional<Value *> SimplifiedAssociatedValue;
 };
 

 struct AAValueSimplifyFloating : AAValueSimplifyImpl {
   AAValueSimplifyFloating(const IRPosition &IRP) : AAValueSimplifyImpl(IRP) {}
 
   /// See AbstractAttribute::initialize(...).
   void initialize(Attributor &A) override {
     Value &V = getAnchorValue();
 
     // TODO: add other stuffs
-    if (isa<Constant>(V) || isa<UndefValue>(V))
+    if (isa<Constant>(V))
       indicatePessimisticFixpoint();
   }
 
   /// See AbstractAttribute::updateImpl(...).
   ChangeStatus updateImpl(Attributor &A) override {
     bool HasValueBefore = SimplifiedAssociatedValue.hasValue();
 
     auto VisitValueCB = [&](Value &V, BooleanState, bool Stripped) -> bool {
       auto &AA = A.getAAFor<AAValueSimplify>(*this, IRPosition::value(V));