Page MenuHomePhabricator

Add AutoUpgrade function to add new address space datalayout string to existing datalayouts.

Authored by akhuang on Sep 16 2019, 1:07 PM.



Add function to AutoUpgrade to change the datalayout of old X86 datalayout strings.
This adds "-p270:32:32-p271:32:32-p272:64:64" to X86 datalayouts that are otherwise valid
and don't already contain it.

This also removes the compatibility changes in
Datalayout change in

Diff Detail


Event Timeline

akhuang created this revision.Sep 16 2019, 1:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 1:07 PM
echristo accepted this revision.Sep 16 2019, 1:13 PM

Thanks! Idea sounds great, I'm out on vacation so I'll ask rnk to review :)

This revision is now accepted and ready to land.Sep 16 2019, 1:13 PM
akhuang updated this revision to Diff 220384.Sep 16 2019, 1:43 PM

Add test cases

rnk added inline comments.Sep 17 2019, 2:30 PM
3583 ↗(On Diff #220384)

This assumes that the triple always appears before the data layout. The writer always puts them in the expected order, but just to be safe, I'd move this out of the while (true) loop.

4122 ↗(On Diff #220384)

I would use this code pattern to leverage the Triple parsing logic:

Triple::ArchType Arch = Triple(TT).getArch();
if (Arch != llvm::Triple::x86 && Arch != llvm::Triple::x86_64 ...)

Otherwise this won't fire for an i686-* triple.

4126 ↗(On Diff #220384)

What about -p:64:64 for x64?

5–6 ↗(On Diff #220384)

I think we should use the x64 datalayout with the x64 triple, and then add a separate test for an i686 triple & data layout.

akhuang marked 4 inline comments as done.Sep 17 2019, 5:06 PM
akhuang added inline comments.
4126 ↗(On Diff #220384)

I was basing the pattern off of the computeDataLayout function in X86TargetMachine.cpp, which doesn't seem to add -p:64:64. There are a bunch of tests that have -p:64:64:64 in the DL, but I think those wouldn't pass the DL assert anyway if they were in a test case where it matters?

akhuang updated this revision to Diff 220591.Sep 17 2019, 5:07 PM
  • Address comments
rnk added inline comments.Sep 18 2019, 11:06 AM
4126 ↗(On Diff #220384)

Oh, I see, this is only for x32-like targets.

4132–4134 ↗(On Diff #220591)

This looks like use-after-return. I think you can do the concatenation and call .str() on the result to get a std::string, which is safe to return.

6–7 ↗(On Diff #220591)

You know, it occurs to me that these might be better as unit tests. The prototype of llvm::UpgradeDataLayout takes and returns strings, so it would be really easy to do these in gtest. I think it's actually worth taking the time to do that.

We might also want to write some negative tests for cases that we don't want to upgrade.

akhuang updated this revision to Diff 220748.Sep 18 2019, 2:01 PM
akhuang marked an inline comment as done.
  • Add unittests
  • Change return type to std::string
akhuang marked an inline comment as done.Sep 18 2019, 2:18 PM
akhuang added inline comments.
6–7 ↗(On Diff #220591)

I added a unittest file with some upgrade tests and some negative tests.

rnk accepted this revision.Sep 18 2019, 2:52 PM


akhuang updated this revision to Diff 220758.Sep 18 2019, 3:13 PM

clang format

This revision was automatically updated to reflect the committed changes.
arichardson added inline comments.

Why was virtual removed here? We override this in the MIPS backend for our CHERI fork.

If the intention is to not allow targets to override this, then the documentation also needs to be adjusted.
However, I doubt avoiding the virtual function call gives any measurable performance speedup.

rnk added inline comments.Nov 17 2019, 8:51 AM

I dug through the history trying to figure out why it was made virtual, and wasn't able to find any strong reason for it. Obviously, no in tree targets use it. If you want to add it back, I'd approve such a patch.

Also, I seem to recall that overriding this wasn't actually enough for our use cases anyway, so it didn't seem that useful.

arichardson added inline comments.Nov 17 2019, 9:37 AM

Looking through our history I was not able to see why we added the override. It was added during a merge from upstream with no explanation. I just removed the override and all tests still pass so it no longer appears necessary. Sorry for the noise.

Although I guess the This hook provides a target specific check on the validity of this DataLayout. comment should probably be deleted.