This is an archive of the discontinued LLVM Phabricator instance.

Make DataLayout Non-Optional in the Module
ClosedPublic

Authored by mehdi_amini on Mar 1 2015, 10:04 PM.

Details

Summary

DataLayout keeps the string used for its creation.

As a side effect it is no longer needed in the Module.
This is "almost" NFC, the string is no longer
canonicalized, you can't rely on two "equals" DataLayout
having the same string returned by getStringRepresentation().

Get rid of DataLayoutPass: the DataLayout is in the Module

The DataLayout is "per-module", let's enforce this by not
duplicating it more than necessary.
One more step toward non-optionality of the DataLayout in the
module.

Make DataLayout Non-Optional in the Module

Module->getDataLayout() will never returns nullptr anymore.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Make DataLayout Non-Optional in the Module.
mehdi_amini updated this object.
mehdi_amini edited the test plan for this revision. (Show Details)
mehdi_amini added a subscriber: Unknown Object (MLST).

Three separate commits that make DataLayout non-optional in the Module.

The next step will be to update all the passes and trash the dead code that
handle missing DataLayout.

Forgot to clang-format...

echristo edited edge metadata.Mar 2 2015, 9:40 AM

Few small comments:

a) You've got Function::getDataLayout - I think I'd prefer people to generally have to ask the Function parent to get the data layout. (It's also a little inconsistent in the code, but should be fairly sed'able).

b) Routines to construct a data layout (Totally a follow up patch) in llvm rather than requiring each front end to handle construct them up themselves.

c) Relatedly you don't do anything about the code in the backends that constructs the data layouts?

-eric

mehdi_amini edited edge metadata.

Rebased on http://reviews.llvm.org/D8027

I.e: don't use getDataLayout() anywhere else than on the Module itself.

(clang-format)

In general it's looking pretty good. Couple more comments inline and one other which is "You pass a datalayout on the command line in some testcases and put it in the module in others. Any reason for the disparity?"

Thanks!

-eric

test/Transforms/InstCombine/overflow-mul.ll
180

Can't put this at the top? Also, might want to explain why.

test/Transforms/InstCombine/store.ll
44

Inquiring minds here on the swap?

test/Transforms/LoopStrengthReduce/X86/2011-07-20-DoubleIV.ll
7

This comment (repeated in a number of testcases) is a little oddly worded. Perhaps:

"Provide legal integer types."?

-eric

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

test/Transforms/InstCombine/overflow-mul.ll
180

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.

test/Transforms/InstCombine/store.ll
44

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.

test/Transforms/LoopStrengthReduce/X86/2011-07-20-DoubleIV.ll
7

I'll update this.

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

-eric

test/Transforms/InstCombine/load-cmp.ll
1

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.

test/Transforms/InstCombine/overflow-mul.ll
180

OK. That's just weird.

test/Transforms/InstCombine/store.ll
44

Again with a yuck, might be worth filing a bug or something on this.

echristo accepted this revision.Mar 4 2015, 12:21 PM
echristo edited edge metadata.

Oh, I don't recall a matching clang patch and you'll need one since it sets up a pass manager. With that ready to go then it should be fine.

Thanks!

-eric

This revision is now accepted and ready to land.Mar 4 2015, 12:21 PM
mehdi_amini closed this revision.Mar 8 2015, 10:09 PM
test/Transforms/PhaseOrdering/scev.ll