[PATCH] D66967: [Attributor] ValueSimplify Abstract Attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2447
+    // If everyting is clear, reach optimistic fixpoint.
+    return ExistNone ? ChangeStatus::UNCHANGED : indicateOptimisticFixpoint();
+  }
----------------
jdoerfert wrote:
> Why unchanged? Don't we have to indicate *any* change in the state? (Same below)
I'm unsure about this. Even if `None` was not observed we could still lose the information if it was assumed, right? I would prefer to be careful also wrt. to future extensions that could break some invariant here.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2496
+    // TODO: Implement this for phi, GEP, and etc.
+    return indicatePessimisticFixpoint();
+  }
----------------
jdoerfert wrote:
> `GenericValueTraversal` time ;)
I think the `AAValueSimplifyImpl::manifest` should be fine here once the attribute specialization is gone. You can test it with some phi tests for example.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2373
+  static bool checkAndUpdate(Attributor &A, const AbstractAttribute &QueryingAA,
+                             Value &V, bool &ExistNone,
+                             Optional<Value *> &SimplifiedValue) {
----------------
Can we add more description for the parameters here and maybe rename `ExistNone` to something more descriptive wrt. what that means.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2413
+        }
+      }
+    }
----------------
Why check for arguments and why check for users? Just replace all users of `V` with `C` or do I miss sth?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2431
+      indicatePessimisticFixpoint();
+  }
+
----------------
No need for this check, I think. we always have an associated function for arguments and even if the definition is not exact, we can use information from the call sites to improve this definition.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2465
+      indicatePessimisticFixpoint();
+  }
+
----------------
AA...Returned always has an associated function.


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

https://reviews.llvm.org/D66967