Page MenuHomePhabricator

[VE] Target-specific bit size for sjljehprepare
ClosedPublic

Authored by simoll on Dec 11 2019, 3:03 AM.

Details

Summary

This patch extends the TargetMachine to let targets specify the integer size used by the sjljehprepare pass. This is 64bit for the VE target and otherwise defaults to 32bit for all targets, which was hard-wired before.

Diff Detail

Event Timeline

simoll created this revision.Dec 11 2019, 3:03 AM

To clarify: this patch is not intended to be committed as-is but gives an overview of the required changes. It's the a patch of a series for the VE backend and includes all the earlier patches.

simoll updated this revision to Diff 233330.Dec 11 2019, 5:33 AM

Trimmed down to SjLj changes only.

simoll added a project: Restricted Project.Dec 11 2019, 5:44 AM
simoll updated this revision to Diff 237035.Jan 9 2020, 5:45 AM
  • Rebased
  • NFC (getArch() == .. -- > isVE()).
  • Dug through layers of cosmetic commits to find potential reviewers for the SjLjEHPreparePass.
simoll updated this revision to Diff 239239.Jan 21 2020, 12:30 AM
simoll edited the summary of this revision. (Show Details)
simoll added a reviewer: arsenm.

Rebased

Unit tests: pass. 62043 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

arsenm added inline comments.Jan 21 2020, 5:12 AM
llvm/lib/CodeGen/TargetPassConfig.cpp
698–699

I would expect this to be a target hook of some kind the pass checks, not a pass parameter controlled here. Checking a target triple here is probably not kosher

simoll planned changes to this revision.Jan 21 2020, 5:48 AM
simoll marked an inline comment as done.
simoll added inline comments.
llvm/lib/CodeGen/TargetPassConfig.cpp
698–699

Ok. How about querying the SjLj data size from TTI (eg TTI::getSjLjDataSize())?

arsenm added inline comments.Jan 21 2020, 5:55 AM
llvm/lib/CodeGen/TargetPassConfig.cpp
698–699

I think this probably belongs in either the TargetMachine or TargetLowering. I don't think things required for correctness should go in TTI

simoll updated this revision to Diff 239309.Jan 21 2020, 6:46 AM
simoll marked 2 inline comments as done.
  • Rebased
  • Moved data size query to TargetMachine

Unit tests: pass. 62057 tests passed, 0 failed and 784 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

simoll updated this revision to Diff 239318.Jan 21 2020, 7:19 AM
  • Rebased
  • Style pass

Unit tests: pass. 62057 tests passed, 0 failed and 784 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

simoll planned changes to this revision.Jan 31 2020, 7:35 AM
  • test
simoll updated this revision to Diff 245124.Feb 18 2020, 4:41 AM
simoll retitled this revision from [VE,#0] 64bit data for SjLj to [VE] Target-specific bit size for sjljehprepare.
simoll edited the summary of this revision. (Show Details)
simoll added a reviewer: k-ishizaka.
  • initialize SjljEHPrepare pass with CodeGen library.
  • rebased
  • added test (32bit on X86, 64bit on VE).
arsenm added inline comments.Feb 24 2020, 11:19 AM
llvm/test/CodeGen/VE/sjlj_except.ll
2–3

You should be able to test this with just opt running the pass? You certainly don't need to rely on -print-after

14

I don't know anything about SJLJ but I would guess it's possible to shrink this test more?

33

Weird indent?

simoll planned changes to this revision.Feb 24 2020, 5:33 PM
simoll marked 2 inline comments as done.

Thanks for having a look :)

llvm/test/CodeGen/VE/sjlj_except.ll
2–3

I was hoping to do that. However, when the pass is created without a target machine (as happens in opt) - it defaults to the standard bit size. Since VE is the only target that actually uses a non-default value, i found no other way to test it than to go all the way to VE codegen.

14

Probably. I'll look into it.

simoll updated this revision to Diff 247575.Mar 2 2020, 2:08 AM
simoll marked 2 inline comments as done.
  • rebased
  • simplified test

ping. Any more comments for this one?

arsenm accepted this revision.Mar 10 2020, 8:09 AM

Maybe this should go in TLI instead, but it's not really important

This revision is now accepted and ready to land.Mar 10 2020, 8:09 AM
This revision was automatically updated to reflect the committed changes.