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

[AIX] Handle toc-data offset overflowing 16-bits #80092

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

syzaara
Copy link
Contributor

@syzaara syzaara commented Jan 31, 2024

When the toc-data offset overflows the 16-bits, we can truncate the value to the 16-bit value as the linker will handle overflow through fixup code.

@llvmbot llvmbot added the mc Machine (object) code label Jan 31, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 31, 2024

@llvm/pr-subscribers-backend-powerpc

@llvm/pr-subscribers-mc

Author: Zaara Syeda (syzaara)

Changes

When the toc-data offset overflows the 16-bits, we can truncate the value to the 16-bit value as the linker will handle overflow through fixup code.


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

2 Files Affected:

  • (modified) llvm/lib/MC/XCOFFObjectWriter.cpp (+19-4)
  • (added) llvm/test/CodeGen/PowerPC/aix-overflow-toc-data.py (+59)
diff --git a/llvm/lib/MC/XCOFFObjectWriter.cpp b/llvm/lib/MC/XCOFFObjectWriter.cpp
index 343e2fc877bc3..1a8ae755307f9 100644
--- a/llvm/lib/MC/XCOFFObjectWriter.cpp
+++ b/llvm/lib/MC/XCOFFObjectWriter.cpp
@@ -718,11 +718,26 @@ void XCOFFObjectWriter::recordRelocation(MCAssembler &Asm,
     } else {
       // The FixedValue should be the TOC entry offset from the TOC-base plus
       // any constant offset value.
-      const int64_t TOCEntryOffset = SectionMap[SymASec]->Address -
-                                     TOCCsects.front().Address +
-                                     Target.getConstant();
+      int64_t TOCEntryOffset = SectionMap[SymASec]->Address -
+                               TOCCsects.front().Address + Target.getConstant();
+      // For small code model, if the TOCEntryOffset overflows the 16-bit value,
+      // we truncate it back down to 16 bits. The linker will be able to insert
+      // fix-up code when needed.
+      // For non toc-data symbols, we already did the truncation in
+      // PPCAsmPrinter.cpp through setting Target.getConstant() in the
+      // expression above by calling getTOCEntryLoadingExprForXCOFF for the
+      // various TOC PseudoOps.
+      // For toc-data symbols, we were not able to calculate the offset from
+      // the TOC in PPCAsmPrinter.cpp since the TOC has not been finalized at
+      // that point, so we are adjusting it here though
+      // llvm::SignExtend64<16>(TOCEntryOffset);
+      // TODO: Since the time that the handling for offsets over 16-bits was
+      // added in PPCAsmPrinter.cpp using getTOCEntryLoadingExprForXCOFF, the
+      // system assembler and linker have been updated to be able to handle the
+      // overflowing offsets, so we no longer need to keep
+      // getTOCEntryLoadingExprForXCOFF.
       if (Type == XCOFF::RelocationType::R_TOC && !isInt<16>(TOCEntryOffset))
-        report_fatal_error("TOCEntryOffset overflows in small code model mode");
+        TOCEntryOffset = llvm::SignExtend64<16>(TOCEntryOffset);
 
       FixedValue = TOCEntryOffset;
     }
diff --git a/llvm/test/CodeGen/PowerPC/aix-overflow-toc-data.py b/llvm/test/CodeGen/PowerPC/aix-overflow-toc-data.py
new file mode 100644
index 0000000000000..d271eb4fedeec
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/aix-overflow-toc-data.py
@@ -0,0 +1,59 @@
+# UNSUPPORTED: expensive_checks, debug
+
+# RUN: %python %s > %t.ll
+# RUN: llc -mtriple powerpc-ibm-aix-xcoff -code-model=small -mcpu=pwr4 -mattr=-altivec -O0 < %t.ll | \
+# RUN:   FileCheck --check-prefix=ASM32 %s
+
+# RUN: llc -mtriple powerpc64-ibm-aix-xcoff -code-model=small -mcpu=pwr4 -mattr=-altivec -O0 < %t.ll | \
+# RUN:   FileCheck --check-prefix=ASM64 %s
+
+# RUN: llc -mtriple powerpc-ibm-aix-xcoff -code-model=small -mcpu=pwr4 -mattr=-altivec -O0 \
+# RUN:     -filetype=obj -o %t.o < %t.ll
+# RUN: llvm-objdump --no-print-imm-hex -D -r --symbol-description %t.o | FileCheck --check-prefix=DIS32 %s
+
+# RUN: llc -mtriple powerpc64-ibm-aix-xcoff -code-model=small -mcpu=pwr4 -mattr=-altivec -O0 \
+# RUN:     -filetype=obj -o %t.o < %t.ll
+# RUN: llvm-objdump --no-print-imm-hex -D -r --symbol-description %t.o | FileCheck --check-prefix=DIS64 %s
+
+numentries = 8195
+for x in range(0, numentries):
+    print("@a%d = global i32 0, align 4 #0" % (x))
+
+print("define void @foo() {")
+print("entry:")
+for x in range(0, numentries):
+    print("store i32 1, i32* @a%d, align 4" % (x))
+print("ret void")
+print("}")
+
+print("attributes #0 = { \"toc-data\" }")
+
+# 32-bit assembly check
+# ASM32:  la 4, a0[TD](2)
+# ASM32:  la 4, a1[TD](2)
+
+# ASM32:  la 4, a8191[TD](2)
+# ASM32:  la 4, a8192[TD](2)
+# ASM32:  la 4, a8193[TD](2)
+
+# 64-bit assembly check
+# ASM64:  la 4, a0[TD](2)
+# ASM64:  la 4, a1[TD](2)
+
+# ASM64:  la 4, a8191[TD](2)
+# ASM64:  la 4, a8192[TD](2)
+# ASM64:  la 4, a8193[TD](2)
+
+# DIS32:    fffc: 38 82 7f fc   addi 4, 2, 32764
+# DIS32:      0000fffe:  R_TOC  (idx: 16391) a8191[TD]
+# DIS32:   10004: 38 82 80 00   addi 4, 2, -32768
+# DIS32:      00010006:  R_TOC  (idx: 16393) a8192[TD]
+# DIS32:   1000c: 38 82 80 04   addi 4, 2, -32764
+# DIS32:      0001000e:  R_TOC  (idx: 16395) a8193[TD]
+
+# DIS64:    fffc: 38 82 7f fc   addi 4, 2, 32764
+# DIS64:      0000fffe:  R_TOC  (idx: 16391) a8191[TD]
+# DIS64:   10004: 38 82 80 00   addi 4, 2, -32768
+# DIS64:      00010006:  R_TOC  (idx: 16393) a8192[TD]
+# DIS64:   1000c: 38 82 80 04   addi 4, 2, -32764
+# DIS64:      0001000e:  R_TOC  (idx: 16395) a8193[TD]

Copy link

github-actions bot commented Jan 31, 2024

✅ With the latest revision this PR passed the Python code formatter.


# RUN: llc -mtriple powerpc-ibm-aix-xcoff -code-model=small -mcpu=pwr4 -mattr=-altivec -O0 \
# RUN: -filetype=obj -o %t.o < %t.ll
# RUN: llvm-objdump --no-print-imm-hex -D -r --symbol-description %t.o | FileCheck --check-prefix=DIS32 %s
Copy link
Contributor

Choose a reason for hiding this comment

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

Zaara Syeda added 3 commits March 11, 2024 14:48
When the toc-data offset overflows the 16-bits, we
can truncate the value to the 16-bit value as the linker
will handle overflow through fixup code.
Copy link
Contributor

@amy-kwan amy-kwan left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comment. I do not have any additional comments so unless there are some from others, LGTM.

// added in PPCAsmPrinter.cpp using getTOCEntryLoadingExprForXCOFF, the
// system assembler and linker have been updated to be able to handle the
// overflowing offsets, so we no longer need to keep
// getTOCEntryLoadingExprForXCOFF.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the situation that all supported OS levels would now have an assembler that will produce the expected results without the workaround or is it just new OS levels have the fix and we have to still wait until OS levels with the old assemblers drop off the supported list over time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Further investigation is needed to verify the various supported OS levels to check if the fix is available. We can do that separately to this patch and open up follow up patch to remove the truncation in PPCAsmPrinter.cpp once verified.

# UNSUPPORTED: expensive_checks, debug

# RUN: %python %s > %t.ll
# RUN: llc -mtriple powerpc-ibm-aix-xcoff -code-model=small -mcpu=pwr4 -mattr=-altivec -O0 < %t.ll | \
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe for new tests we target pwr7 which is the default arch for AIX.

@syzaara syzaara merged commit 6582509 into llvm:main Mar 28, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:PowerPC mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants