[PATCH] D122287: [XCOFF] support writing sections, relocations and symbols for XCOFF64.

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Looks almost good to me. Thanks for adding this support.



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:74
   } break;
+  case PPC::fixup_ppc_half16ds:
+  case PPC::fixup_ppc_half16dq: {
----------------
Is it possible to add some tests to verify the new relocation types added here for 64-bit objects, especially the ones the existing case changes do not contain?


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:84
+    case MCSymbolRefExpr::VK_PPC_L:
+      return {XCOFF::RelocationType::R_TOCL, 15};
+    }
----------------
Should we handle `RelocationType::R_TOCU`? I guess it may be generated in large code model(`-mcmodel=large`).


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:82
+    case MCSymbolRefExpr::VK_None:
+      return {XCOFF::RelocationType::R_TOC, 15};
+    case MCSymbolRefExpr::VK_PPC_L:
----------------
Esme wrote:
> DiggerLin wrote:
> > DiggerLin wrote:
> > > Esme wrote:
> > > > DiggerLin wrote:
> > > > > I am not sure, whether I understand correct or not ?
> > > > > 1. fixup_ppc_half16ds: A 14-bit fixup corresponding to lo16(_foo) with implied 2 zero bits for instrs like 'std'.
> > > > > 2. fixup_ppc_half16dq:  A 16-bit fixup corresponding to lo16(_foo) with implied 3 zero bits for instrs like 'lxv'.
> > > > > 3. fixup_ppc_half16 : A 16-bit fixup corresponding to lo16(_foo) or ha16(_foo) for instrs like 'li' or 'addis'.
> > > > > 
> > > > > in the https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__sua3i125jbau 
> > > > > 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.
> > > > > 
> > > > > so for the
> > > > > case PPC::fixup_ppc_half16ds: 
> > > > >     {XCOFF::RelocationType::R_TOC, EncodedSignednessIndicator | 13 } 
> > > > Thanks for your catch!
> > > PPC::fixup_ppc_half16dq: is a A 16-bit fixup corresponding to lo16(_foo) with implied 3 zero bits for instrs like 'lxv'. 
> > > for  PPC::fixup_ppc_half16dq 
> > > it should be 
> > > {XCOFF::RelocationType::R_TOC, EncodedSignednessIndicator | 15 } ?
> > I am not a expert on the relocation, I just put my question and we may be need someone who is expert on the relocation to review the code.
> > 
> ```
>   /// A 16-bit fixup corresponding to lo16(_foo) with implied 3 zero bits for
>   /// instrs like 'lxv'. Produces the same relocation as fixup_ppc_half16ds.
>   PPC::fixup_ppc_half16dq
> ```
> Thanks, I thinks I need more investigation about this, the comment says it produces the same relocation as fixup_ppc_half16ds.
Hmm, yes, I also think this is a little confuse. From 64bit OpenPower ABI, section 3.5.1, it says:
> In the following figure, half16ds is similar to half16, but is really just 14 bits because the two least significant bits must be zero and are not really part of the field. (Used by, for example, the ldu instruction.) In addition to the use of this relocation field with the DS forms, half16ds relocations are also used in conjunction with DQ forms. In those instances, the linker and assembler collaborate to create valid DQ forms. They raise an error if the specified offset does not meet the constraints of a valid DQ instruction form displacement.

I think it clearly indicates that `fixup_ppc_half16ds` and `fixup_ppc_half16dq` should use same relocation infos.

For the bit length, it does not give a clear answer, it says ` is similar to half16, but is really just 14 bits`. I checked the source code of commercial XLC, it gives 15. So I think it should be good to use 15 here.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-extern-weak.ll:351
+; CHECKSYM64-NEXT:   Symbol {
+; CHECKSYM64-NEXT:     Index: 1
+; CHECKSYM64-NEXT:     Name: .foo_ext_weak
----------------
Use relative index like above `Index: [[#Index+25]]`?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-extern.ll:480
+; CHECKSYM64-NEXT:     CSECT Auxiliary Entry {
+; CHECKSYM64-NEXT:       Index: 8
+; CHECKSYM64-NEXT:       SectionLen: 0
----------------
Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122287