[PATCH] D44043: [DAGCombine] Remove AND in SETCC if we can prove they are unneeded

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks all for the reviews. I'm currently hoping that the transform Sanjay suggested will take away my main motivation for this patch. Here are some responses in any case:

> I was asked not to add DemandedElts to individual cases in computeKnownBits until you have test coverage - this should probably be the same for AnyToZeroExtLoads.

OK. Noted. By this you mean unit tests for computeKnownBits? Sounds like a sensible idea. I'm guessing the nature of ISel makes this difficult?

> Random idea - would a more general approach be for us to compute undefined bits as well as one/zero bits - either adding them to the KnownBits struct or add it separately?

In this specific case (SETCC(AND(%x, C) , AND(%y, C)) where we know that the ~C bits of %x == %y, then the AND can be removed) - we need to know the bits are a specific value. We can't have the two sides being undef and then choosing to differently zero/signext for example. In the case I was looking, x and y were both and(xor(8bit load, -1), 255), so the top bits are all 1 on both sides, so long as we choose to zeroext. I think we need to know the bits, and know which nodes to change to force those bits to be specified. It might well be a good idea for other situations.



================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1385
+  computeKnownBits(SDValue Op, KnownBits &Known,
+                   SmallPtrSetImpl<LoadSDNode *> *AnyToZeroExtLoads = nullptr,
+                   unsigned Depth = 0) const;
----------------
samparker wrote:
> If you reorder the default parameters, you could reduce the number of changes needed in the backends.
Yeah, makes sense. That's probably best. I was trying to keep depth as the last argument for some reason.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:2682
+          LD->getAlignment(), LD->getMemOperand()->getFlags(), LD->getAAInfo());
+      DAG.ReplaceAllUsesOfValueWith(SDValue(LD, 0), Load);
+      if (LD == N0LHS.getNode())
----------------
samparker wrote:
> Is this safe? Shouldn't the load be checked for a single use or that all the uses on your known bits path?
I don't know a huge amount about the internals of selection dag. My understanding was that if they are anyext then we cannot presume the values of the bits, but then neither can anything else. So long as we set the values of the bits (as we do here), then all other optimisations have to then use the bits as 0.

If I could have just changed the existing LD's to zeroextends, that would be simpler. But I believe the nodes are immutable.

So I don't think we have to check for single uses as the only bits we are changing are the upper bits from unspecified to 0's. Which isn't something that anything else can rely upon, without itself doing the unspecified -> set transform.


https://reviews.llvm.org/D44043