[PATCH] D77808: [SCCP] Use conditional info with AND/OR branch conditions.

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM modulo some naming.



================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1268
       Value *CmpOp1 = Cmp->getOperand(1);
-      if (CopyOf != CmpOp0 && CopyOf != CmpOp1) {
-        mergeInValue(ValueState[&CB], &CB, OriginalVal);
+      // Bail out if neither of the operands matches the OriginalVal or CopyOf.
+      if (CmpOp0 != OriginalVal && CmpOp1 != OriginalVal) {
----------------
The "or CopyOf" is too much now. (You might want to convert this into an assertion, to make sure there are no more surprises here.)


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1296
       ValueLatticeElement &IV = ValueState[&CB];
-      if (CondVal.isConstantRange() || OriginalVal.isConstantRange()) {
-        auto NewCR =
+      ValueLatticeElement CopyOfVal = getValueState(CopyOf);
+      if (CondVal.isConstantRange() || CopyOfVal.isConstantRange()) {
----------------
You already fetched this above under the name of `OriginalValState`.

I think the naming here got a bit messed up due to the back and forth refactoring. You might want to rename `OriginalValState` above to `CopyOfVal`, to avoid confusing with `OriginalVal`. I'd also s/OriginalVal/OriginalOp to keep with the name from PredicateInfo, and because you're using `Val` to indicate lattice values rather than `Value*`s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77808