[PATCH] D19087: [x86] prefer comparisons against zero for and+cmp sequences

[Sanjay Patel via llvm-commits]

spatel added a comment.

In http://reviews.llvm.org/D19087#422972, @kbsmith1 wrote:

> Did you add tests to check that lt/gt conditions don't get transformed?


Oops - let me add those and update the patch. The EQ/NE check is hopefully more obvious in the code now.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:1336
@@ +1335,3 @@
+  // x86 or 'rlwinm' on PPC).
+  assert(!valueHasExactlyOneBitSet(Y, DAG) &&
+         "Expected a value with one bit set to already be transformed");
----------------
kbsmith1 wrote:
> Although this might be your expectation, I don't think it should be an assertion.  I think it should be
> if (!valueHasExactlyOneBitSet(Y, DAG))
>   return SDVALUE();
> 
> Using an assertion could cause a compiler internal error during a compilation.  There is a safe and correct return by using SDValue(), and this code shouldn't really need to know whether the transform for bit test should happen before or after it.
I made this an assert rather an actual condition because I'm assuming this helper function should be tightly coupled with "SimplifySetCC()" below, and I thought duplicating a call to computeKnownBits() (by way of valueHasExactlyOneBitSet()) would be considered wasteful. 

The reason I've broken this into a separate function is because it felt wrong to add more code directly to SimplifySetCC() - that is already approaching 1000 lines.



http://reviews.llvm.org/D19087