[PATCH] Make DataLayout Non-Optional in the Module

[Eric Christopher]

Couple of inline comments. Otherwise I think this looks fine to me.

-eric


================
Comment at: test/Transforms/InstCombine/load-cmp.ll:1
@@ -1,3 +1,2 @@
-; RUN: opt -instcombine -S < %s | FileCheck -check-prefix=NODL %s
-; RUN: opt -instcombine -S -default-data-layout="p:32:32:32-p1:16:16:16-n8:16:32:64" < %s | FileCheck -check-prefix=P32 %s
+; RUN: opt -instcombine -S -default-data-layout="p:32:32:32-p1:16:16:16-n8:16:32:64" < %s | FileCheck %s
 
----------------
This is the one I meant about default-data-layout, but I see what you mean. No reason to put it in the file other than being able to remove the option, but that's not necessary so OK.

================
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"
----------------
joker.eph wrote:
> echristo wrote:
> > 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
> ```
> 
> into 
> 
> ```
> %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.
OK. That's just weird.

================
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
----------------
joker.eph wrote:
> echristo wrote:
> > 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.
Again with a yuck, might be worth filing a bug or something on this.

http://reviews.llvm.org/D7992

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/