[PATCH] D79625: [PowerPC] Extend .reloc directive on PowerPC

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/include/llvm/MC/MCExpr.h:305
     VK_PPC_NOTOC,           // symbol at notoc
+    VK_PPC_LINKER_OPT,      // Not actually emitted as symbol at ppclinkeropt
 
----------------
I think the name should include `PCREL` since this Expr is for representing the PCREL_OPT relocation.

The other comment show you how to produce the variant kind. What about `//  .reloc expr, R_PPC64_PCREL_OPT, expr` for the comment? 


================
Comment at: llvm/lib/MC/MCExpr.cpp:325
   case VK_PPC_NOTOC: return "notoc";
+  case VK_PPC_LINKER_OPT: return "ppclinkeropt";
   case VK_COFF_IMGREL32: return "IMGREL";
----------------
nemanjai wrote:
> stefanp wrote:
> > amyk wrote:
> > > I noticed for other PPC VariantKinds, we don't put the ppc part in the string. Is there a reason why we call it `ppclinkeropt` instead of just `linkeropt`? Is it since linkeropt might sound too generic?
> > This variant kind is a little bit different from the others. Other variant kinds use the string when generating assembly output files. For example when we use VK_PPC_GOT_PCREL we actually output `symbol at got@pcrel` into the assembly file. This string has to match what gcc does when it emits the symbol and relocation in the output. In the case of VK_PPC_LINKER_OPT we won't be doing that. The string "ppclinkeropt" should never get printed to the assembly file so it doesn't really matter what we name it as long as it is consistent in LLVM. I used the "ppc" prefix for it because the strings needs to be unique (see `getVariantKindForName`) and as a result I figured it would be less likely to conflict with a future string.
> It seems to me that `<<Invalid>>` may be the right thing to do to clearly signal that this thing isn't supposed to be emitted. Is there something else (other than `getVariantKindForName`) that needs this to have some value such as the one you chose?
Is the reason the string needs to be unique because its used in a sting switch in `getVariantKindForName`? If so mapping it to VK_Invalid in both here and there like Nemanja suggests seems like the right thing to do.


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1811
+/// ParseDirectiveReloc
+/// Very similar to the one in AsmParser::parseDirectiveReloc except that we
+/// want to be able to parse MCExpr::Binary too.
----------------
Was the problem that the existing functionality can only parse a constant or symbol reference for the offset?  If so why extend the functionality in the PPCASMParser as opposed to  `AsmParser::parseDirectiveReloc`? I'm not super familiar with the .reloc directive but gas documents accepting symbol + offset for the first expression. https://sourceware.org/binutils/docs/as/Reloc.html#Reloc


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

https://reviews.llvm.org/D79625