Page MenuHomePhabricator

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

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

Details

Summary

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 https://reviews.llvm.org/D66843.
Datalayout change in https://reviews.llvm.org/D64931.

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
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3583

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.

llvm/lib/IR/AutoUpgrade.cpp
4122

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

What about -p:64:64 for x64?

llvm/test/Bitcode/upgrade-datalayout.ll
5–6

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.
llvm/lib/IR/AutoUpgrade.cpp
4126

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
llvm/lib/IR/AutoUpgrade.cpp
4126

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

4132–4134

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.

llvm/test/Bitcode/upgrade-datalayout2.ll
7–8

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.
llvm/test/Bitcode/upgrade-datalayout2.ll
7–8

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

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

lgtm

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.
llvm/trunk/include/llvm/Target/TargetMachine.h
160 ↗(On Diff #220759)

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
llvm/trunk/include/llvm/Target/TargetMachine.h
160 ↗(On Diff #220759)

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
llvm/trunk/include/llvm/Target/TargetMachine.h
160 ↗(On Diff #220759)

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.