[PATCH] D68721: [NFC][PowerPC]Clean up PPCAsmPrinter for TOC related pseudo opcode

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added a comment.

LGTM with some straightforward changes (can be fixed on the commit).



================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:774
     if (GlobalToc || MO.isJTI() || MO.isBlockAddress() ||
-        TM.getCodeModel() == CodeModel::Large)
+	(MO.isCPI() && TM.getCodeModel() == CodeModel::Large))
       MOSymbol = lookUpOrCreateTOCEntry(MOSymbol);
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > Xiangling_L wrote:
> > > hubert.reinterpretcast wrote:
> > > > This does not seem to be an NFC change.
> > > Since Machine Operand for this TOC pseudo opcode can only be `MO.isGlobal() || MO.isCPI() || MO.isJTI() || MO.isBlockAddress()`, so the line`(MO.isCPI() && TM.getCodeModel() == CodeModel::Large)` will have the same logic with or without `MO.isCPI()`. But since people @jsji have some slight preference to make it clear, I add `MO.isCPI()`. Please let me know if you still have any concerns about this change.
> > I do not believe that the statement is sufficiently supported in this context except if there is:
> > ```
> > assert(GlobalToc || !MO.isGlobal());
> > ```
> > 
> I think I kinda know your concern, do you mean without my changes, for query `if (GlobalToc || MO.isJTI() || MO.isBlockAddress() || TM.getCodeModel() == CodeModel::Large)`, there is the possibility that `GlobalToc` is false, but the code model is large, under which we still generate TOCEntry for global MO? If my understanding is correct, then I think we don't need to worry about this situation, because according to the definition of `isGVIndirectSymbol`:
> 
> ```
> bool PPCSubtarget::isGVIndirectSymbol(const GlobalValue *GV) const {
>   // Large code model always uses the TOC even for local symbols.
>   if (TM.getCodeModel() == CodeModel::Large)
>     return true;
>   if (TM.shouldAssumeDSOLocal(*GV->getParent(), GV))
>     return false;
>   return true;
> }
> 
> ```
> 
> When it's large code model,  and `MO.isGlobal() == true`, `GlobalToc` is definitely `true`, so there is no situation that when `MO.isGlobal()` &&`GlobalToc == false`,  but the code model is not large. 
> 
Okay, thanks.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:687
     // Map the operand to its corresponding MCSymbol.
-    MCSymbol *MOSymbol = nullptr;
-    if (MO.isGlobal())
-      MOSymbol = getSymbol(MO.getGlobal());
-    else if (MO.isCPI())
-      MOSymbol = GetCPISymbol(MO.getIndex());
-    else if (MO.isJTI())
-      MOSymbol = GetJTISymbol(MO.getIndex());
-    else if (MO.isBlockAddress())
-      MOSymbol = GetBlockAddressSymbol(MO.getBlockAddress());
-
+    const MCSymbol* const MOSymbol = getMCSymbolForTOCPseudoMO(MO, *this);
     const bool IsAIX = TM.getTargetTriple().isOSAIX();
----------------
My fault on the previous comment; please adjust the space: `const MCSymbol *const MOSymbol`.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1379
 
-    for (MapVector<MCSymbol*, MCSymbol*>::iterator I = TOC.begin(),
-         E = TOC.end(); I != E; ++I) {
-      OutStreamer->EmitLabel(I->second);
-      MCSymbol *S = I->first;
+    for (const auto &I : TOC) {
+      OutStreamer->EmitLabel(I.second);
----------------
I prefer to use meaningful names:
```
for (const auto &TOCMapPair : TOC) {
  const MCSymbol *const TOCEntryTarget = TOCMapPair.first;
  MCSymbol *const TOCEntryLabel = TOCMapPair.second;

  OutStreamer->EmitLabel(TOCEntryLabel);
  if (isPPC64) {
    TS.emitTCEntry(*TOCEntryTarget);
  } else {
    OutStreamer->EmitValueToAlignment(4);
    OutStreamer->EmitSymbolValue(TOCEntryTarget, 4)
  }
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68721