This is an archive of the discontinued LLVM Phabricator instance.

[COFF][X86_64] Put jump table in .rdata for Windows
ClosedPublic

Authored by wxiao3 on Feb 23 2023, 11:18 PM.

Details

Summary

Put jump table in .rdata for Windows to align with that for Linux.
It can avoid loading the same code page into I$ and D$ simultaneously
and thus favor performance.

Diff Detail

Event Timeline

wxiao3 created this revision.Feb 23 2023, 11:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 11:18 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
wxiao3 requested review of this revision.Feb 23 2023, 11:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 11:18 PM
pengfei added inline comments.Feb 25 2023, 5:40 AM
llvm/test/MC/COFF/jumptable-rdata.ll
1

You may add a RUN to check the option.

LuoYuanke added inline comments.Feb 25 2023, 5:33 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
74

Not sure if it worth to have a feature tuning for this.

wxiao3 updated this revision to Diff 500529.Feb 26 2023, 2:26 AM

RUN to check the added option

wxiao3 marked an inline comment as done.Feb 26 2023, 2:26 AM
ph0b added a subscriber: ph0b.Feb 27 2023, 9:49 AM
wxiao3 added inline comments.Feb 27 2023, 6:24 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
74

I don't think tuning is needed here because It's not related to specific processor.

LuoYuanke added inline comments.Feb 27 2023, 6:27 PM
llvm/test/MC/COFF/jumptable-rdata.ll
3

The default value of jumptable-in-function-section is false, so in this test should we set "jumptable-in-function-section" to 1?

wxiao3 added inline comments.Feb 27 2023, 6:36 PM
llvm/test/MC/COFF/jumptable-rdata.ll
3

yes, the CHECK-OPT test set "jumptable-in-function-section" to 1 already.

This revision is now accepted and ready to land.Feb 27 2023, 6:38 PM

Note that this behaviour isn't entirely strictly uniform across all COFF targets. See D57277, where the AArch64 target was changed to move jumptables on COFF to the text section. This change was then later reversed in D113576 when the AArch64/COFF target supported the relevant relocations.

Before this change, COFF on x86_32 and aarch64 put the jumptables in rdata, while x86_64 and arm (use thumbv7 in triples passed to llc) put them in the text section (in the case of arm, the jumptable is emitted inline in the function and not after it). This patch seems to change the output of x86_64, but doesn't affect the others. I think it would be good to clarify this aspect in the commit message and wording. I'm mildly concerned about how it affects other targets when this is universal code for COFF, but at least on a quick test it didn't make any difference for the arm case (the only one where the jump tables would be left in the text section).

The testcase here seems a bit brittle; if I run this testcase with an aarch64-pc-win32 triple, I don't get any jumptable generated at all, since it can mostly be optimized out. See e.g. llvm/test/CodeGen/AArch64/win64-jumptable.ll for a less brittle testcase.

wxiao3 updated this revision to Diff 501069.Feb 28 2023, 3:16 AM
  1. only affect x86
  2. less brittle testcase

Note that this behaviour isn't entirely strictly uniform across all COFF targets. See D57277, where the AArch64 target was changed to move jumptables on COFF to the text section. This change was then later reversed in D113576 when the AArch64/COFF target supported the relevant relocations.

Before this change, COFF on x86_32 and aarch64 put the jumptables in rdata, while x86_64 and arm (use thumbv7 in triples passed to llc) put them in the text section (in the case of arm, the jumptable is emitted inline in the function and not after it). This patch seems to change the output of x86_64, but doesn't affect the others. I think it would be good to clarify this aspect in the commit message and wording. I'm mildly concerned about how it affects other targets when this is universal code for COFF, but at least on a quick test it didn't make any difference for the arm case (the only one where the jump tables would be left in the text section).

The testcase here seems a bit brittle; if I run this testcase with an aarch64-pc-win32 triple, I don't get any jumptable generated at all, since it can mostly be optimized out. See e.g. llvm/test/CodeGen/AArch64/win64-jumptable.ll for a less brittle testcase.

Thanks for the comments! I have refactored the patch accordingly.

Thanks for the updates!

On second look, I'm a bit undecided and divided whether it's better to have the arch switch, or just keep it as it was (or include aarch64 in the switch too?). I guess it's fine with me either way. (And the aarch64 and arm cases should have their own testcases already, I believe.)

For the testcase, can you include i386 or i686 too, since that's at least covered in the switch?

wxiao3 retitled this revision from [COFF] Put jump table in .rdata for Windows to [COFF][X86] Put jump table in .rdata for Windows.Feb 28 2023, 3:26 AM
wxiao3 updated this revision to Diff 501108.Feb 28 2023, 5:40 AM

restrict the change only to x86_64

wxiao3 retitled this revision from [COFF][X86] Put jump table in .rdata for Windows to [COFF][X86_64] Put jump table in .rdata for Windows.Feb 28 2023, 5:41 AM

Thanks for the updates!

On second look, I'm a bit undecided and divided whether it's better to have the arch switch, or just keep it as it was (or include aarch64 in the switch too?). I guess it's fine with me either way. (And the aarch64 and arm cases should have their own testcases already, I believe.)

For the testcase, can you include i386 or i686 too, since that's at least covered in the switch?

Restrict the change only to x86_64 for 2 reasons:

  1. can't put jump table to text section for x86_32 since its jump table entry kind is EK_BlockAddress instead of EK_LabelDifference32
  2. no idea about the performance impact to aarch64

Thanks for the updates!

On second look, I'm a bit undecided and divided whether it's better to have the arch switch, or just keep it as it was (or include aarch64 in the switch too?). I guess it's fine with me either way. (And the aarch64 and arm cases should have their own testcases already, I believe.)

For the testcase, can you include i386 or i686 too, since that's at least covered in the switch?

Restrict the change only to x86_64 for 2 reasons:

  1. can't put jump table to text section for x86_32 since its jump table entry kind is EK_BlockAddress instead of EK_LabelDifference32
  2. no idea about the performance impact to aarch64

Ok, thanks, that makes sense. Then no further comments from me. If the other reviewers already were ok with it, I'd say it's good to go - thanks!

This revision was landed with ongoing or failed builds.Feb 28 2023, 6:36 PM
This revision was automatically updated to reflect the committed changes.