[PATCH] D13470: Teach computeKnownBits to use new align attribute/metadata

[Philip Reames via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/comments addressed and one part separated into a follow on change as noted inline.


================
Comment at: lib/Analysis/ValueTracking.cpp:1412
@@ -1411,3 +1411,3 @@
 
-static unsigned getAlignment(Value *V, const DataLayout &DL) {
+static unsigned getAlignment(const Value *V, const DataLayout &DL) {
   unsigned Align = 0;
----------------
The changes to this function LGTM w/appropriate test cases.

================
Comment at: lib/Analysis/ValueTracking.cpp:2981
@@ -2980,3 @@
-    BaseAlign = AI->getAlignment();
-  else if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(Base))
-    BaseAlign = GV->getAlignment();
----------------
I'm not sure the GlobalVariable code is equivalent to the other case.  I would prefer you separate the refactoring to use getAlignment into a separate patch.  It seems slightly higher risk.  

(My concern comes from the difference between BaseAlign and Align.  I haven't looked at these carefully and I'm worried that your getAlignment is computing Align not BaseAlign.)

================
Comment at: test/Transforms/InstCombine/assume-redundant.ll:52
@@ +51,3 @@
+
+; Check that redundant align assume is removed
+; CHECK-LABEL: @test
----------------
Please add versions of this test for both load w/metadata and allocas.  


http://reviews.llvm.org/D13470