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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 13 inline comments as done.
Xiangling_L 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());
----------------
hubert.reinterpretcast wrote:
> Has a switch on `getType` been considered?
Sure.


================
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);
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:836
+    LLVM_DEBUG(
+        !(MO.isGlobal() && Subtarget->isGVIndirectSymbol(MO.getGlobal())) &&
+        "Interposable definitions must use indirect access.");
----------------
hubert.reinterpretcast wrote:
> Missing the invocation of `assert` here?
Thank you for mentioning this mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68721