[PATCH] D65802: [DAGCombiner] Rebuild (setcc x, y, ==) from (xor (xor x, y), 1)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13496-13499
+  // Transform (brcond (xor x, y)) -> (brcond (setcc, x, y, ne))
+  // Transform (brcond (xor (xor x, y), -1)) -> (brcond (setcc, x, y, eq))
+  if (N.getOpcode() == ISD::XOR && N->hasOneUse() &&
+      (*N->use_begin())->getOpcode() == ISD::BRCOND) {
----------------
rogfer01 wrote:
> lebedev.ri wrote:
> > rogfer01 wrote:
> > > lebedev.ri wrote:
> > > > Why is this one-use restriction being introduced here?
> > > There are two callers of this function `visitSETCC`, which passes a node generated by `SimplifySetCC`, and `visitBRCOND` which does indeed check that the passed node has just one use.
> > > 
> > > I'm less sure about the the node coming out of `SimplifySetCC` so it might make sense to err on the safe side here?
> > `(*N->use_begin())->getOpcode() == ISD::BRCOND)` should go, or make it an assert.
> > 
> > I'm not really sure what's on the other unsafe side, against what is this protecting?
> > 
> @lebedev.ri @spatel I removed the "one use" check but I'm under the impression that we still want the assert, do we?
Hm, no, i would think the assert should go away too.
Since there's no longer one-use check, we can't assume that `ISD::BRCOND` is he first user.
And i'm not sure against what we'd guarding with that check.


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

https://reviews.llvm.org/D65802