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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:517
+static MCSymbol *getMCSymbolForTOCPseudoMO(const MachineOperand &MO, AsmPrinter &AP) {
+  if (MO.isGlobal())
+    return AP.getSymbol(MO.getGlobal());
----------------
Has a switch on `getType` been considered?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:685
     // 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 *MOSymbol = getMCSymbolForTOCPseudoMO(MO, *this);
     const bool IsAIX = TM.getTargetTriple().isOSAIX();
----------------
Is it also possible to use `* const`?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:739
+    assert((MO.isGlobal() || MO.isCPI() || MO.isJTI() || MO.isBlockAddress()) &&
+            "Invalid operand!");
 
----------------
There's an extra space in the indentation here.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:760
+    // Change the opcode to ADDIS8. If the global address is the address of
+    // an external symbol, is a jump table address, is a block address or is a
+    // constant pool index with large code model enabled, then generate a TOC
----------------
Please don't remove the Oxford comma before the "or".


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:771
+
+    bool GlobalToc = 
+	MO.isGlobal() && Subtarget->isGVIndirectSymbol(MO.getGlobal());
----------------
Can this be `const`?


================
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);
----------------
This does not seem to be an NFC change.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:797
+    // an external symbol, is a jump table address, is a block address, or is
+    // a constant pool index with large code model enabled then generate a
+    // TOC entry and reference that. Otherwise reference the symbol directly.
----------------
Comma before "then".


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:798
+    // a constant pool index with large code model enabled then generate a
+    // TOC entry and reference that. Otherwise reference the symbol directly.
     TmpInst.setOpcode(PPC::LD);
----------------
Comma after "Otherwise".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68721