[PATCH] Make DataLayout Non-Optional in the Module

[Mehdi Amini]

> On Mar 4, 2015, at 12:20 PM, Eric Christopher <echristo at gmail.com> wrote:
> 
> 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.

I’m not sure about the real value of the default-data-layout command line option, so I’m not against remove the option entirely.
But I just didn’t care and assumed that the the option still make sense, and since we have an option I thought we better have a test for it.


>> 
>> 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.

What exactly bugs you here? 
I mean, I’m not sure what to put in the bug report?

Thanks,

— 
Mehdi