[PATCH] D85632: [Attributor] Implement AAPotentialValues and connect it with AAValueSimplify

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think we should limit the "entry point" for this AA. It seems we can only improve AAValueSimplify when the instruction we look at is a conditional, right? So if it is, we ask AAPotentialValues, otherwise we do not. Other users of AAPotentialValues will emerge but for this use we can restrict it to the case we can actually fold the multiple values in to a single constant. Other users might be OK with multiple values, e.g., to determine a common property.



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7343
+                                       const APInt &LHS, const APInt &RHS,
+                                       bool &Valid) {
+    Instruction::BinaryOps BinOpcode = BinOp->getOpcode();
----------------
Add a TODO that we should look at `nsw` and `nuw` to help, e.g., to exclude certain results that cannot happen without creating `undef`. Similarly, division by `undef` should not make the result invalid but simply skip this operand pair.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7358
+    case Instruction::Mul:
+      return LHS * RHS;
+    case Instruction::UDiv:
----------------
Why do we have the copy sometimes and sometimes not? Are the operators missing in the APInt class? If so, we should fix that in a pre-patch first.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7406
+    auto &LHSAA = A.getAAFor<AAPotentialValues>(*this, IRPosition::value(*LHS));
+    auto &RHSAA = A.getAAFor<AAPotentialValues>(*this, IRPosition::value(*RHS));
+
----------------
Here and below we should avoid looking at RHS if LHSAA is invalid.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7412
+    auto LHSAAPVS = LHSAA.getAssumedSet();
+    auto RHSAAPVS = RHSAA.getAssumedSet();
+
----------------
Please add a type here, and (or at least) make it a const reference. Also other places.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7420
+        MaybeTrue |= CmpResult;
+        MaybeFalse |= !CmpResult;
+      }
----------------
We should give up once both are true.


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

https://reviews.llvm.org/D85632