[PATCH] D72027: [XCOFF][AIX] Support basic relocation type on AIX

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:58
 
+struct XCOFFRelocation {
+  uint32_t SymbolTableIndex;
----------------
hubert.reinterpretcast wrote:
> I just want to make sure we have a good reason (e.g., a sizable performance benefit) for having this in-memory version instead of reusing the `llvm::object::XCOFFRelocation32` serialization version.
As the comment above suggested, we do not want to use the version in llvm/Object for object file generation.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:406
+
+  uint32_t Index = (SymA.isTemporary() || SymA.isUndefined(false))
+                       ? SymbolIndexMap[SymASec->getQualNameSymbol()]
----------------
hubert.reinterpretcast wrote:
> Is this really about temporary symbols, or is this more to do with linkage?
Whenever we have a temporary symbol, we would relocate its containing csect because temporary symbol would not be on the symbol table. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:699
+
+  // TODO What to align the SymbolTable too?
+  // TODO Error check that the number of symbol table entries fits in 32-bits
----------------
hubert.reinterpretcast wrote:
> It seems there is no special alignment requirement for the symbol table. Alignment as low as 2-byte has been observed.
I will remove the TODO.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:22
 class PPCXCOFFObjectWriter : public MCXCOFFObjectTargetWriter {
+  static constexpr uint8_t SignBitMask = 0x80;
+
----------------
hubert.reinterpretcast wrote:
> There's `XR_SIGN_INDICATOR_MASK` in the codebase already.
Unfortunately that's in llvm/Object. And we should have put it in llvm/BinaryFormat. 


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:71
+
+uint8_t PPCXCOFFObjectWriter::getRelocSignAndSize(const MCFixup &Fixup,
+                                                  bool IsPCRel) const {
----------------
hubert.reinterpretcast wrote:
> There is a chance that merging this with the previous function would improve the logic and reduce parallel maintenance. In particular, the "magic numbers" are easier to swallow if they are paired with the XCOFF relocation type.
Suggestion taken. 


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:86
+  case PPC::fixup_ppc_br24:
+    Result |= 25;
+    break;
----------------
jasonliu wrote:
> DiggerLin wrote:
> > jasonliu wrote:
> > > DiggerLin wrote:
> > > > DiggerLin wrote:
> > > > > // The relocation encodes the bit length being relocated minus 1. Add back the 1 to // get the actual length being relocated.
> > > > > this one should be 23 ?
> > > > And my suggestion is to use a constexpr instead of using number directly here.
> > > No, we have 25 in the actual object file. 
> > the  bit length of the relocatable reference of PPC::fixup_ppc_br24 is 24 bits ?
> > if it is , according to xcoff document .
> > r_rsize : 0x3F(6 bits)
> > Specifies the bit length of the relocatable reference minus one. The current architecture allows for fields of up to 32 bits (XCOFF32) or 64 bits (XCOFF64) to be relocated.
> > 
> > it should be 24bits -1 = 23.
> Branches are 4 byte aligned, so the 24 bits we encode in the instruction actually represents a 26 bit offset (because the implication the bottom 2 bits are 0), so it is 26-1.
Regarding to your suggestion for using constexpr value here... I'm not sure if it really helps that much. As the magic number I'm using here actually have some relationship with the Fixup Kind, in my mind, it's actually easier for people to reason about why we use that number if we just put that number in directly. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll:125
+; DIS-NEXT:       14: 60 00 00 00                   nop
+; DIS-NEXT:       18: 80 82 00 00                   lwz 4, 0(2)
+; DIS-NEXT:       1c: 80 84 00 00                   lwz 4, 0(4)
----------------
hubert.reinterpretcast wrote:
> It would be better if there was a case where the TOC entry is at a non-zero offset from the TOC base.
You are right. I added the test and discovered we also need to calculate the FixedValue for R_TOC relocation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72027/new/

https://reviews.llvm.org/D72027