[XCOFF][AIX] Handle TOC entries that could not be reached by positive range in small code model

[jasonliu]

Summary

In small code model, AIX assembler could not deal with labels that could not be reached within the [-0x8000, 0x8000) range from TOC base.
So when generating the assembly, we would need to help the assembler by subtracting an offset from the label to keep the actual value within [-0x8000, 0x8000).

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

603

Can we add an assertion here to make sure EntryDistanceFromTOCBase - PositiveTOCRange do not go beyond -0x8000?

llvm/test/CodeGen/PowerPC/aix-overflow-toc.test

39

Since toc entry is 8 bytes under 64bit mode, does that mean we can only have no more than 8192 entries?

[jasonliu]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

603

EntryDistanceFromTOCBase - PositiveTOCRange is always going to be a positive number. So not sure why we would need an assertion for it to be >= 0x8000?

llvm/test/CodeGen/PowerPC/aix-overflow-toc.test

39

The compiler and assembler could still generate more than 8192 entries. But if you need to link it, you would need -bbigtoc for the link to be successful. Basically, if there are more than 8192 entries, we would need to rely on linker to generate branch out code to make everything work.

[jasonliu]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

603

Sorry, I mean >= -0x8000

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

588

The assertion here seems a bit unnecessary, since lookUpOrCreateTOCEntry will always return us a TOCEntry.

593

s/need/needs

597

s/enties/entries;
s/could represented/could be represented or you can remove "could".

603

Sorry for the confusion, I meant Expr -TOCRange * Multiply. And I think your reply below has clarified that there is no such assertion needed.

llvm/test/CodeGen/PowerPC/aix-overflow-toc.test

33

nit: trailing whitespace.

[jasonliu]

Address comments.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

588

I put the assertion here as a defensive mechanism in case the code get move around in the future and someone calls this function before lookUpOrCreateTOCEntry.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-overflow-toc.test

1

Noting that most tests that use the test file itself as a Python script are named with a py file extension (e.g., llvm/test/MC/COFF/bigobj.py).

[jasonliu]

llvm/test/CodeGen/PowerPC/aix-overflow-toc.test

1

Yeah... I noticed that.
But for some reason, when I initially use .py as file extension, it results in lit not running that test when running a folder of tests (It's fine if I only run that test) . Could be that I need to tweak some lit config file to make it not skip over? But I didn't get to the bottom of it, and used '.test' as workaround.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-overflow-toc.test

1

Something like llvm/test/CodeGen/SystemZ/Large/lit.local.cfg would could be placed into llvm/test/CodeGen/PowerPC/. Indeed, llvm/test/MC/COFF/bigobj.py doesn't seem to run.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

590

Is this an int or is this more a ptrdiff_t?

592

This could be constexpr?

595

Since the TOC entries are multiples of 4 or 8, both the < and <= results are the same; however, the <= version matches the semantics better.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

598–607

I'd like to see a "cleanly derived" version of this for general offsets from the TOC-base virtual address. Also, this part should be pulled into another lambda (taking an input expression and associated offset) that generally handles adjustment of TOC relocation expressions.

For example, the result should be 0 when using an offset value from 0 to 32768 (inclusive) and an offset of 98303 should produce 1 instead of 2.

Part of my rationale is that it is a "code smell" that the current calculation does not work in general. Another factor is that TOC-related accesses at non-zero offsets from the beginning of the entry are meaningful. The TD storage mapping class is used to store data within the TOC (as opposed to storing a pointer to the data in the TOC). We should be able to reuse the calculation for the general-offset case when we need it.

It's probably easier to start with the comment:
We're not just keeping the offset to be within some range. We are causing the modified notional offset from the TOC base (to be encoded into the instruction's D or DS field) to be the signed 16-bit truncation of the original notional offset from the TOC base. This is consistent with the treatment used both by XL C/C++ and by AIX ld -r.

Solving for the adjustment value to subtract:
(int16_t)original_offset = original_offset - adjustment
(int16_t)original_offset + adjustment = original_offset
adjustment = original_offset - (int16_t)original_offset

But we're dealing with C++14 code, so (with the hopes that SignExtend32 is sufficiently well written):
adjustment = original_offset - llvm::SignExtend32<16>(original_offset)

[jasonliu]

Address review comments.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

598–607

Thanks. Please let me know if my new revision does not address your concern.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

585–589

Adjust the comment to fit the context.

595

How would this relate to XCOFFObjectWriter::recordRelocation?

I notice that there's a line there:

FixedValue = SectionMap[SymASec]->Address - TOCCsects.front().Address;

Does the new expression formed here mean that we should account for it through Target.getConstant()?

609–614

Since the adjustment is now factored out, the comment doesn't quite fit. Indeed, the code would read fine without a comment if the condition was reversed to make it obvious when special handling is needed.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-overflow-toc.py

3 ↗

(On Diff #291029)

Indentation does not match with line 5.

[jasonliu]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

595

From the testing I have done, there seems to be no particular action need to be taken there (except removing that report_fatal_error in there), as the value we put into the instruction could already wrap around naturally when it goes out of the range.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

595

I think we should add the Target.getConstant() because it could show up for using the TD storage mapping class or maybe for integrated assembly. The report_fatal_error enforces a constraint that the no wraparound is necessary at that stage. As in, we may want the object-writing path to require input that would also work on the assembly-writing path.

[jasonliu]

Address comments.

[hubert.reinterpretcast]

LGTM with minor comments.

llvm/lib/MC/XCOFFObjectWriter.cpp

433 ↗

(On Diff #291095)

Update the comment to match the code change.

llvm/test/CodeGen/PowerPC/aix-overflow-toc.py

64 ↗

(On Diff #291095)

Minor nit: Indentation.

[fhahn]

It appears this patch added a test that is failing with expensive checks on GreenDragon: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/17604/console. It would be great if you could take a look and revert if it takes longer to fix.

[hubert.reinterpretcast]

In D86879#2273354, @fhahn wrote:

It appears this patch added a test that is failing with expensive checks on GreenDragon: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/17604/console. It would be great if you could take a look and revert if it takes longer to fix.

It says:

Timeout: Reached timeout of 600 seconds

We believe that the test is necessarily large. Has the associated bot encountered similar issues before? What was the recommended fix to retain the test in more appropriate environments?

[nemanjai]

In D86879#2274025, @hubert.reinterpretcast wrote:

We believe that the test is necessarily large. Has the associated bot encountered similar issues before? What was the recommended fix to retain the test in more appropriate environments?

Perhaps the test case should be restricted to some key platforms where cross-compilation to AIX is relevant? Perhaps Linux (PPC is already required).

[jasonliu]

In D86879#2274076, @nemanjai wrote:

In D86879#2274025, @hubert.reinterpretcast wrote:

We believe that the test is necessarily large. Has the associated bot encountered similar issues before? What was the recommended fix to retain the test in more appropriate environments?

Perhaps the test case should be restricted to some key platforms where cross-compilation to AIX is relevant? Perhaps Linux (PPC is already required).

I could add
# REQUIRES: aix || linux
to the test if that helps.

[fhahn]

In D86879#2274099, @jasonliu wrote:

In D86879#2274076, @nemanjai wrote:

In D86879#2274025, @hubert.reinterpretcast wrote:

We believe that the test is necessarily large. Has the associated bot encountered similar issues before? What was the recommended fix to retain the test in more appropriate environments?

Perhaps the test case should be restricted to some key platforms where cross-compilation to AIX is relevant? Perhaps Linux (PPC is already required).

I could add
# REQUIRES: aix || linux
to the test if that helps.

The problem here is with expensive checks, which makes it take too long I think. It works fine on Darwin bots without expensive checks. If it is not possible to reduce the size of the test, then we need to disable it at least for the configuration used for http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive. I am not sure if there's another way to force a big gap in PPC assembly.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

for (const MachineOperand &MO: MI->operands()) {
         } else {
           if (PPC::SPERCRegClass.contains(Reg))
             llvm_unreachable("SPE register found in FPU-targeted code!");
         }
       }
     }
   }
 #endif
+
+  auto getTOCEntryLoadingExprForXCOFF =
+      [IsPPC64, this](const MCSymbol *MOSymbol,
+                      const MCExpr *Expr) -> const MCExpr * {
+    const unsigned EntryByteSize = IsPPC64 ? 8 : 4;
+    const auto TOCEntryIter = TOC.find(MOSymbol);
+    assert(TOCEntryIter != TOC.end() &&
+           "Could not find the TOC entry for this symbol.");
+    const int EntryDistanceFromTOCBase =
+        (TOCEntryIter - TOC.begin()) * EntryByteSize;
+    const int16_t PositiveTOCRange = INT16_MAX;
+    // Nothing need to be done when TOC entries are still within positive range.
+    if (EntryDistanceFromTOCBase < PositiveTOCRange)
+      return Expr;
+
+    // AIX system assembler could not directly deal with TOC enties that are not
+    // within the range that could represented by an int16_t type in small code
+    // model mode. But it works happily if we keep the offset of those TOC
+    // entries within that range.
+    const int TOCRange = 0x10000;
+    const int Multiply =
+        (EntryDistanceFromTOCBase - PositiveTOCRange) / TOCRange + 1;
+    return MCBinaryExpr::createAdd(
+        Expr, MCConstantExpr::create(-TOCRange * Multiply, OutContext),
+        OutContext);
+  };
+
   // Lower multi-instruction pseudo operations.
   switch (MI->getOpcode()) {
   default: break;
   case TargetOpcode::DBG_VALUE:
     llvm_unreachable("Should be handled target independently");
   case TargetOpcode::STACKMAP:
     return LowerSTACKMAP(SM, *MI);
   case TargetOpcode::PATCHPOINT:
     return LowerPATCHPOINT(SM, *MI);
 
   case PPC::MoveGOTtoLR: {
     // Transform %lr = MoveGOTtoLR
     // Into this: bl _GLOBAL_OFFSET_TABLE_@local-4
     // _GLOBAL_OFFSET_TABLE_@local-4 (instruction preceding
case PPC::LWZtoc: {
 
     // AIX uses the label directly as the lwz displacement operand for
     // references into the toc section. The displacement value will be generated
     // relative to the toc-base.
     if (IsAIX) {
       assert(
           TM.getCodeModel() == CodeModel::Small &&
           "This pseudo should only be selected for 32-bit small code model.");
+      Exp = getTOCEntryLoadingExprForXCOFF(MOSymbol, Exp);
       TmpInst.getOperand(1) = MCOperand::createExpr(Exp);
       EmitToStreamer(*OutStreamer, TmpInst);
       return;
     }
 
     // Create an explicit subtract expression between the local symbol and
     // '.LTOC' to manifest the toc-relative offset.
     const MCExpr *PB = MCSymbolRefExpr::create(
case PPC::LDtoc: {
 
     // Change the opcode to LD.
     TmpInst.setOpcode(PPC::LD);
 
     const MachineOperand &MO = MI->getOperand(1);
     assert((MO.isGlobal() || MO.isCPI() || MO.isJTI() || MO.isBlockAddress()) &&
            "Invalid operand!");
 
+    // Map the operand to its corresponding MCSymbol.
+    const MCSymbol *const MOSymbol = getMCSymbolForTOCPseudoMO(MO, *this);
+
     // Map the machine operand to its corresponding MCSymbol, then map the
     // global address operand to be a reference to the TOC entry we will
     // synthesize later.
-    MCSymbol *TOCEntry =
-        lookUpOrCreateTOCEntry(getMCSymbolForTOCPseudoMO(MO, *this));
+    MCSymbol *TOCEntry = lookUpOrCreateTOCEntry(MOSymbol);
 
     const MCSymbolRefExpr::VariantKind VK =
         IsAIX ? MCSymbolRefExpr::VK_None : MCSymbolRefExpr::VK_PPC_TOC;
     const MCExpr *Exp =
         MCSymbolRefExpr::create(TOCEntry, VK, OutContext);
-    TmpInst.getOperand(1) = MCOperand::createExpr(Exp);
+    TmpInst.getOperand(1) = MCOperand::createExpr(
+        IsAIX ? getTOCEntryLoadingExprForXCOFF(MOSymbol, Exp) : Exp);
     EmitToStreamer(*OutStreamer, TmpInst);
     return;
   }
   case PPC::ADDIStocHA: {
     assert((IsAIX && !IsPPC64 && TM.getCodeModel() == CodeModel::Large) &&
            "This pseudo should only be selected for 32-bit large code model on"
            " AIX.");
 
if (M.empty())
     return;
 
   // Switch to section to emit TOC base.
   OutStreamer->SwitchSection(getObjFileLowering().getTOCBaseSection());
 
   PPCTargetStreamer *TS =
       static_cast<PPCTargetStreamer *>(OutStreamer->getTargetStreamer());
 
-  const unsigned EntryByteSize = Subtarget->isPPC64() ? 8 : 4;
-  const unsigned TOCEntriesByteSize = TOC.size() * EntryByteSize;
-  // TODO: If TOC entries' size is larger than 32768, then we run out of
-  // positive displacement to reach the TOC entry. We need to decide how to
-  // handle entries' size larger than that later.
-  if (TOCEntriesByteSize > 32767) {
-    report_fatal_error("Handling of TOC entry displacement larger than 32767 "
-                       "is not yet implemented.");
-  }
-
   for (auto &I : TOC) {
     // Setup the csect for the current TC entry.
     MCSectionXCOFF *TCEntry = cast<MCSectionXCOFF>(
         getObjFileLowering().getSectionForTOCEntry(I.first, TM));
     OutStreamer->SwitchSection(TCEntry);
 
     OutStreamer->emitLabel(I.second);
     if (TS != nullptr)

llvm/test/CodeGen/PowerPC/aix-overflow-toc.test

+# 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
+
+numentries = 12290
+for x in range(0, numentries):
+    print("@a%d = global i32 0, align 4" % (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("}")
+
+# 32-bit assembly check
+# ASM32:  lwz 3, L..C0(2)
+# ASM32:  lwz 3, L..C1(2)
+
+# ASM32:  lwz 3, L..C8191(2)
+# ASM32:  lwz 3, L..C8192-65536(2)
+# ASM32:  lwz 3, L..C8193-65536(2)
+
+# ASM32:  lwz 3, L..C12288-65536(2)
+# ASM32:  lwz 3, L..C12289-65536(2)
+
+# 64-bit assembly check
+# ASM64:  ld 3, L..C0(2)
+# ASM64:  ld 3, L..C1(2)
+
+# ASM64:  ld 3, L..C4095(2) 
+# ASM64:  ld 3, L..C4096-65536(2)
+# ASM64:  ld 3, L..C4097-65536(2)
+
+# ASM64:  ld 3, L..C12287-65536(2)
+# ASM64:  ld 3, L..C12288-131072(2)
+# ASM64:  ld 3, L..C12289-131072(2)