Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[X86] Place data in large sections for large code model #70265

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

aeubanks
Copy link
Contributor

@aeubanks aeubanks commented Oct 25, 2023

This allows better interoperability mixing small/medium/large code model
code since large code model data can be put into separate large sections.

And respect large data threshold under large code model.
gcc also does this: https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html.

See https://groups.google.com/g/x86-64-abi/c/jnQdJeabxiU.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 25, 2023

@llvm/pr-subscribers-backend-x86

Author: Arthur Eubanks (aeubanks)

Changes

This allows better interoperability mixing small/medium/large code model
code since small code model data won't be mixed with large code model
data.


Full diff: https://github.com/llvm/llvm-project/pull/70265.diff

2 Files Affected:

  • (modified) llvm/lib/Target/TargetMachine.cpp (+13-6)
  • (modified) llvm/test/CodeGen/X86/code-model-elf-sections.ll (+5-2)
diff --git a/llvm/lib/Target/TargetMachine.cpp b/llvm/lib/Target/TargetMachine.cpp
index 45fb612cb91da19..e11355de2eed3b5 100644
--- a/llvm/lib/Target/TargetMachine.cpp
+++ b/llvm/lib/Target/TargetMachine.cpp
@@ -42,13 +42,20 @@ TargetMachine::~TargetMachine() = default;
 bool TargetMachine::isLargeData(const GlobalVariable *GV) const {
   if (getTargetTriple().getArch() != Triple::x86_64 || GV->isThreadLocal())
     return false;
-  // Large data under the large code model still needs to be thought about, so
-  // restrict this to medium.
-  if (getCodeModel() != CodeModel::Medium)
+
+  switch (getCodeModel()) {
+  case CodeModel::Large: {
+    return true;
+  }
+  case CodeModel::Medium: {
+    const DataLayout &DL = GV->getParent()->getDataLayout();
+    uint64_t Size = DL.getTypeSizeInBits(GV->getValueType()) / 8;
+    return Size == 0 || Size > LargeDataThreshold;
+  }
+  default: {
     return false;
-  const DataLayout &DL = GV->getParent()->getDataLayout();
-  uint64_t Size = DL.getTypeSizeInBits(GV->getValueType()) / 8;
-  return Size == 0 || Size > LargeDataThreshold;
+  }
+  }
 }
 
 bool TargetMachine::isPositionIndependent() const {
diff --git a/llvm/test/CodeGen/X86/code-model-elf-sections.ll b/llvm/test/CodeGen/X86/code-model-elf-sections.ll
index fe659fa9a46e727..5d02fed05814039 100644
--- a/llvm/test/CodeGen/X86/code-model-elf-sections.ll
+++ b/llvm/test/CodeGen/X86/code-model-elf-sections.ll
@@ -7,14 +7,17 @@
 ; RUN: llc < %s -relocation-model=pic -filetype=obj -code-model=medium -large-data-threshold=80 -o %t
 ; RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=SMALL
 ; RUN: llc < %s -relocation-model=pic -filetype=obj -code-model=large -o %t
-; RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=SMALL
+; RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=LARGE
+; Check that the large code model ignores -large-data-threshold.
+; RUN: llc < %s -relocation-model=pic -filetype=obj -code-model=large -large-data-threshold=800 -o %t
+; RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=LARGE
 
 ; RUN: llc < %s -relocation-model=pic -filetype=obj -code-model=small -data-sections -o %t
 ; RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=SMALL-DS
 ; RUN: llc < %s -relocation-model=pic -filetype=obj -code-model=medium -data-sections -o %t
 ; RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=LARGE-DS
 ; RUN: llc < %s -relocation-model=pic -filetype=obj -code-model=large -data-sections -o %t
-; RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=SMALL-DS
+; RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=LARGE-DS
 
 ; SMALL: .data {{.*}} WA {{.*}}
 ; SMALL: foo {{.*}} WA {{.*}}

@MaskRay
Copy link
Member

MaskRay commented Oct 25, 2023

I have recently changed GCC https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
-mcmodel=large also respects -mlarge-data-threshold=

@MaskRay
Copy link
Member

MaskRay commented Oct 25, 2023

Consider adding a link to https://groups.google.com/g/x86-64-abi/c/jnQdJeabxiU

This allows better interoperability mixing small/medium/large code model
code since small code model data won't be mixed with large code model
data.
@aeubanks aeubanks changed the title [X86] Treat all data under large code model as large [X86] Respect large data threshold under large code model Nov 10, 2023
@aeubanks
Copy link
Contributor Author

I'm still of the opinion that large code model data should never contribute toward relocation pressure, but I'll follow gcc here.

@aeubanks aeubanks changed the title [X86] Respect large data threshold under large code model [X86] Place data in large sections for large code model Nov 15, 2023
@aeubanks
Copy link
Contributor Author

ping

// restrict this to medium.
if (getCodeModel() != CodeModel::Medium)

if (getCodeModel() != CodeModel::Medium && getCodeModel() != CodeModel::Large)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What controls the default for the large data size threshold, under the large code model? I think we discussed making it zero, so effectively all globals will be large data under the large code model, which sounds like the right behavior. I don't know which way GCC went on that.

I think we want to keep the .bss/.data/.rodata prefix checks, even under the large code model. I expect folks to mix large and small code, and we don't want to accidentally make small sections large.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're relying on the frontend to set the large data threshold in TargetMachine, otherwise it's just 0

gcc currently defaults to 65536 as the large data threshold for the large code model

@MaskRay
Copy link
Member

MaskRay commented Nov 17, 2023

I'm still of the opinion that large code model data should never contribute toward relocation pressure, but I'll follow gcc here.

The decision is deliberate. Some users may expect .data.* section name and build features on top of it. These users (say JIT programs) may not care about he small/large code model mixing story.
-mlarge-data-threshold= gives users choices.

@aeubanks aeubanks merged commit 635756e into llvm:main Nov 17, 2023
3 checks passed
sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023
This allows better interoperability mixing small/medium/large code model
code since large code model data can be put into separate large
sections.

And respect large data threshold under large code model.
gcc also does this: https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html.

See https://groups.google.com/g/x86-64-abi/c/jnQdJeabxiU.
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
This allows better interoperability mixing small/medium/large code model
code since large code model data can be put into separate large
sections.

And respect large data threshold under large code model.
gcc also does this: https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html.

See https://groups.google.com/g/x86-64-abi/c/jnQdJeabxiU.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants