[PATCH] Make DataLayout Non-Optional in the Module
mehdi.amini at apple.com
Tue Mar 3 22:01:11 PST 2015
I can't find a single occurrence of the datalayout on the command line in any test, do you have an example?
$ git diff master | grep RUN | grep -i layout
-; RUN: opt < %s -datalayout -instcombine -S | FileCheck %s
-; RUN: opt -instcombine -S -default-data-layout="p:32:32:32-p1:16:16:16-n8:16:32:64" < %s | FileCheck -check-prefix=http://reviews.llvm.org/P32 %s
+; RUN: opt -instcombine -S -default-data-layout="p:32:32:32-p1:16:16:16-n8:16:32:64" < %s | FileCheck %s
I removed the " -datalayout" option which is meaningless now.
And the two other lines is just a rename of the check-prefix.
Comment at: test/Transforms/InstCombine/overflow-mul.ll:180
@@ +179,3 @@
+; Need this weird datalayout to keep the "and i32 %mul, 255" together
+target datalayout = "i32:8:8"
> Can't put this at the top? Also, might want to explain why.
I can put it at the top, but it is only need for this last test, this is why I put it here.
The underlying problem is that InstCombine will see that we manipulate pointers and because the default data layout is "align 4" for pointers, it will know that the lower 3 bits are zero.
It will turn:
%and = and i32 %mul, 255
%and = and i32 %mul, 252
Later when trying to fold to llvm.umul.with.overflow(), it will fail because it expects a mask which is 2^n-1.
This is interesting because this test case is "broken" in the sense that this case would not be folded with "real world" alignment. I plan to address this later by teaching InstCombine to use the known zero bits with the mask.
Comment at: test/Transforms/InstCombine/store.ll:44
@@ -43,3 +43,3 @@
; CHECK: Cont:
-; CHECK-NEXT: %storemerge = phi i32 [ 47, %Cond2 ], [ -987654321, %Cond ]
+; CHECK-NEXT: %storemerge = phi i32 [ -987654321, %Cond ], [ 47, %Cond2 ]
; CHECK-NEXT: ret i32 %storemerge
> Inquiring minds here on the swap?
You are cruel with me, my laziness made me accept this diff without looking further (it seems harrnless).
So I used my lldb skills to find out, and with a DataLayout the alloca is "align 4", and the first store visited is in turn "aligned". It cannot be merge with the other one at this point since they are not identical. It is only when the second store is visited and "aligned" that they can be merged together. Hence the diff.
Note: we could sort deterministically the PHI arguments using something else than "the order the store are visited", for instance using the relative ordering of the basic blocks. I'm not sure it matters that much as the order is still deterministic right now.
Comment at: test/Transforms/LoopStrengthReduce/X86/2011-07-20-DoubleIV.ll:7
@@ -6,1 +6,3 @@
+; Needs to provide legal integer type in the DataLayout
+target datalayout = "n8:16:32:64"
> This comment (repeated in a number of testcases) is a little oddly worded. Perhaps:
> "Provide legal integer types."?
I'll update this.
More information about the llvm-commits