[PATCH] D151799: [ConstraintElim] Try to use first cmp to prove second cmp for ANDs.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:732
+      WorkList.push_back(FactOrCheck::getCheck(DT.getNode(Successor), And));
+  }
 
----------------
nikic wrote:
> Given that this is a check, why is this only handled for branch operands and only if we can add a successor? Shouldn't this be handled the same as icmps in the loop above?
The original intention here was to simplify both the second operand of the `AND` and then also propagate the fact to `true` successors.

I shared D153660 which tracks uses of conditions to simplify, which automatically takes care of that part and allows to greatly simplify this patch.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1053
+        return DTN->getDFSNumIn() >= CB.NumIn &&
+               DTN->getDFSNumOut() <= CB.NumOut;
+      });
----------------
nikic wrote:
> Okay, so I guess this is the bit that makes use of the branch -- but I don't really get what this has to do with the and.
> 
> We're already going to add both and operands as facts for the branch. The only situation where I would expect this to make a difference if an icmp gets defined outside the branch and used inside the branch, and also just happens to be part of a dominating and.
> 
> But this "defined in a different block" situation is a generic problem for constraint elimination, it seems odd to handle it for this specific special case.
> But this "defined in a different block" situation is a generic problem for constraint elimination, it seems odd to handle it for this specific special case.

Agreed, a cleaner way to handle this should be tracking the uses of conditions for simplifications as done in D153660 . That simplifies the code in this patch quite a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151799