[PATCH] D12958: Refactor computeKnownBits alignment handling code

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Analysis/ValueTracking.cpp:1488
@@ -1487,3 @@
-    KnownOne.clearAllBits();
-    return;
-  }
----------------
reames wrote:
> apilipenko wrote:
> > reames wrote:
> > > I'm not sure that removing these returns is actually equivalent.  Does computeKnownBits guarantee to preserve any known bits on entry?  (I don't think so?)  If not, then letting these flow down into the generic code may loose information. 
> > In all existing code paths computeKnownBits starts with cleared bits on both KnownZero and KnownOne. In my patch I just made it explicit by moving the lines which clear both bit sets before the code which handles aligned values. See line 1466.
> I wasn't commenting on the movement of the clearing.  I was commenting on the removal of the return.  
I think this is okay because the functions below that clear known bits are only called if V is a GlobalAlias or an Operator, neither of which is true if V is known to be a GlobalObject (or an Argument).

I agree, however, that the relevant logic here is becoming increasingly unclear. How about we move this alignment logic below the computeKnownBitsFromOperator call, so that it is clear (at least based on existing commentary) that we only ever refine this information? This also has the benefit that we can use an 'else if' and elide some unnecessary dyn_cast checks.



http://reviews.llvm.org/D12958