[llvm-commits] patch: go crazy, compute bits for an entire add instruction

[Chris Lattner]

On Jul 5, 2011, at 11:26 PM, Nick Lewycky wrote:
> This is a patch to ComputeMaskedBits primarily, though it's hard to observe the effect in a small testcase without also adding more smarts someplace else, which this patch does for instsimplify.
> 
> When faced with an add instruction, ComputeMaskedBits will look at the leading bits and the trailing bits to attempt to establish a pattern -- and it does a decent job. However, there may be bits we could calculate in the middle, which we currently don't. Go wild, try to compute every bit in an add.
> 
> This requires a new loop that is O(n) in the number of bits, which I think is sensible enough given that most values will be either 32 or 64 bits, making this effectively a constant-time operation.
> 
> Please review!
> 
> If this patch is okay, I'm going to do it for sub as well obviously, but eventually for multiply. Currently if we transform some things to mul (using mul as a bit-spreading operation, basically) we lose optimization power because we can no longer analyze it as well as the equivalent series of shifts with add's/or's.

Some comments.

- Please find some other testcase to merge sum.ll into.

+++ lib/Analysis/InstructionSimplify.cpp	(working copy)
@@ -1211,6 +1211,21 @@

+    // Are the bits being and'd away already known to be zero?
+    if ((CI->getValue() | KnownZero).isMaxValue())
+      return Op0;

It seems that you should drop this, it can't happen in general because you're not asking ComputeMaskedBits to compute all the bits (similarly the 'or' case).

+  // Check whether the and instruction can be solved through bitwise analysis.
+    APInt KnownZero(BitWidth, 0), KnownOne(BitWidth, 0);
+    ComputeMaskedBits(Op0, APInt::getAllOnesValue(BitWidth),
+                      KnownZero, KnownOne, TD, 0);
+

Here you're asking for all the bits, which is asymmetric with 'and', why?

The code in ValueTracking.cpp is intense enough that you should split it out to its own static helper function.

The logic here is really crazy and I didn't dig into i.  It seems that there should be some ways to simplify the logic though.  For example, you have:

+          (LHSKnownZero[i] && CarryKnownZero) ||
+          (RHSKnownZero[i] && CarryKnownZero)) {

Which can be reassociated.  Also, it seems that there should be some tricky hacker's delight sort of tricks to compute what you want without explicit loops.

I don't have any concrete suggestions though :)

-Chris