[PATCH] D67853: [PowerPC][XCOFF] Fix expansion of LWZtoc Pseudo for AIX.

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L added inline comments.
Herald added a subscriber: wuzish.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:652
   case PPC::LWZtoc: {
-    // Transform %r3 = LWZtoc @min1, %r2
+    // Transform %rN = LWZtoc @op1, %r2
     LowerPPCMachineInstrToMCInst(MI, TmpInst, *this, isDarwin);
----------------
As you mentioned before, should we assert Darwin target is invalid for this toc-related pseudo? Something like:

```
assert (!isDarwin && "TOC is an ELF/XCOFF construct");
```


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:674
+    const bool IsAIX = TM.getTargetTriple().isOSAIX();
+
+    if (PL == PICLevel::SmallPIC && !IsAIX) {
----------------
You seems miss a comment here , which matches the later "Otherwise" on line 684



> // Create a reference to the GOT entry for the symbol. The GOT entry will be synthesized later.






================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:695
+      assert(TM.getCodeModel() == CodeModel::Small &&
+             "This pseudo should only be selected for small code model.");
       TmpInst.getOperand(1) = MCOperand::createExpr(Exp);
----------------
Should we be more specific?
Like:


> This pseudo should only be selected for 32-bit small code model.




================
Comment at: llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll:4
+
+ at b = common dso_local local_unnamed_addr global i32 0, align 4
+ at a = common dso_local local_unnamed_addr global i32 0, align 4
----------------
Personally, I prefer keep the testcase as minimal as possible, so maybe we can remove some attributes like:
"local_unnamed_addr",
"; Function Attrs: norecurse nounwind",
"#0",
"align 4" etc.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67853