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

[wxiao3]

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.

[pengfei]

llvm/test/MC/COFF/jumptable-rdata.ll

2

You may add a RUN to check the option.

[LuoYuanke]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

74

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

[wxiao3]

RUN to check the added option

[wxiao3]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

74

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

[LuoYuanke]

llvm/test/MC/COFF/jumptable-rdata.ll

2

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

[wxiao3]

llvm/test/MC/COFF/jumptable-rdata.ll

2

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

[LuoYuanke]

LGTM.

[mstorsjo]

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]

1. only affect x86
2. less brittle testcase

[wxiao3]

In D144701#4157624, @mstorsjo wrote:

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.

[mstorsjo]

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]

restrict the change only to x86_64

[wxiao3]

In D144701#4157971, @mstorsjo wrote:

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

[mstorsjo]

In D144701#4158272, @wxiao3 wrote:

In D144701#4157971, @mstorsjo wrote:

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!

llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h

MCSection *SelectSectionForGlobal(const GlobalObject *GO, SectionKind Kind,
                                     const TargetMachine &TM) const override;
 
   void getNameWithPrefix(SmallVectorImpl<char> &OutName, const GlobalValue *GV,
                          const TargetMachine &TM) const override;
 
   MCSection *getSectionForJumpTable(const Function &F,
                                     const TargetMachine &TM) const override;
 
+  bool shouldPutJumpTableInFunctionSection(bool UsesLabelDifference,
+                                           const Function &F) const override;
+
   /// Emit Obj-C garbage collection and linker options.
   void emitModuleMetadata(MCStreamer &Streamer, Module &M) const override;
 
   MCSection *getStaticCtorSection(unsigned Priority,
                                   const MCSymbol *KeySym) const override;
   MCSection *getStaticDtorSection(unsigned Priority,
                                   const MCSymbol *KeySym) const override;
 

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

 #include "llvm/Target/TargetMachine.h"
 #include "llvm/TargetParser/Triple.h"
 #include <cassert>
 #include <string>
 
 using namespace llvm;
 using namespace dwarf;
 
+static cl::opt<bool> JumpTableInFunctionSection(
+    "jumptable-in-function-section", cl::Hidden, cl::init(false),
+    cl::desc("Putting Jump Table in function section"));
+
 static void GetObjCImageInfo(Module &M, unsigned &Version, unsigned &Flags,
                              StringRef &Section) {
   SmallVector<Module::ModuleFlagEntry, 8> ModuleFlags;
   M.getModuleFlagsMetadata(ModuleFlags);
 
   for (const auto &MFE: ModuleFlags) {
     // Ignore flags with 'Require' behaviour.
     if (MFE.Behavior == Module::Require)
MCSection *TargetLoweringObjectFileCOFF::getSectionForJumpTable(
   Characteristics |= COFF::IMAGE_SCN_LNK_COMDAT;
   unsigned UniqueID = NextUniqueID++;
 
   return getContext().getCOFFSection(
       SecName, Characteristics, Kind, COMDATSymName,
       COFF::IMAGE_COMDAT_SELECT_ASSOCIATIVE, UniqueID);
 }
 
+bool TargetLoweringObjectFileCOFF::shouldPutJumpTableInFunctionSection(
+    bool UsesLabelDifference, const Function &F) const {
+  if (!JumpTableInFunctionSection) {
+    // We can always create relative relocations, so use another section
+    // that can be marked non-executable.
+    return false;
+  }
+  return TargetLoweringObjectFile::shouldPutJumpTableInFunctionSection(
+    UsesLabelDifference, F);
+}
+
 void TargetLoweringObjectFileCOFF::emitModuleMetadata(MCStreamer &Streamer,
                                                       Module &M) const {
   emitLinkerDirectives(Streamer, M);
 
   unsigned Version = 0;
   unsigned Flags = 0;
   StringRef Section;
 

llvm/test/MC/COFF/jumptable-rdata.ll

+; RUN: llc -mtriple x86_64-pc-win32 < %s | FileCheck %s
+; RUN: llc -mtriple x86_64-pc-win32 -jumptable-in-function-section < %s | FileCheck --check-prefixes=CHECK-OPT %s
+
+define void @switch_jumptable(i32 %what) nounwind {
+entry:
+  switch i32 %what, label %sw.epilog [
+    i32 0, label %sw.bb
+    i32 1, label %sw.bb
+    i32 2, label %sw.bb
+    i32 3, label %sw.bb14
+    i32 4, label %sw.bb18
+    i32 6, label %sw.bb57
+  ]
+
+sw.bb:                                            ; preds = %entry, %entry, %entry
+  ret void
+
+sw.bb14:                                          ; preds = %entry
+  ret void
+
+sw.bb18:                                          ; preds = %entry
+  ret void
+
+sw.bb57:                                          ; preds = %entry
+  ret void
+
+sw.epilog:                                        ; preds = %entry
+  ret void
+}
+; CHECK: .section        .rdata
+; CHECK-OPT-NOT: .section        .rdata