[PATCH] D68341: [AIX] TOC pseudo expansion for 64bit large + 64bit small + 32bit large modes

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:909
+
+    MCSymbol *MOSymbol = getMCSymbolForMO(MO, *this);
 
----------------
nit: MOSymbol could move down a bit to be closer to where it get used. 

Also I would want to add const to this MOSymbol. But it seems that a lot of similar MOSymbols in this file do not have const with them, only add const for this one here would create inconsistency. Not sure if it's worth to have a NFC patch to add const for all the MOSymbol in this file first. 


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:3171
                       (PPCtoc_entry tglobaladdr:$disp, i32:$reg))]>;
-def ADDIStocHA : PPCEmitTimePseudo<(outs gprc:$rD), (ins gprc_nor0:$reg, tocentry32:$disp),
-                       "#ADDIStocHA",
-                       [(set i32:$rD,
-                         (PPCtoc_entry i32:$reg, tglobaladdr:$disp))]>;
+let hasSideEffects = 0, isReMaterializable = 1 in {
+def ADDIStocHA: PPCEmitTimePseudo<(outs gprc:$rD), (ins gprc_nor0:$reg, tocentry32:$disp),
----------------
Curious about what the effect is for adding hasSideEffects and isReMaterializable.
We already have ADDIStocHA before for the other targets, but they did not require hasSideEffects = 0, and isReMaterializable = 1.
So what is special about the AIX target that we need to add them? Or is it simply an omission before and it's actually needed for the other targets as well? 
I try to remove this line here and no test case would fail. If we need those, should we add test case for it?


================
Comment at: llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll:2
 ; RUN: llc -mtriple powerpc-ibm-aix-xcoff \
-; RUN: -code-model=small < %s | FileCheck %s
+; RUN: -code-model=small < %s | FileCheck %s --check-prefix=SMALL
+
----------------
Do we want to add -verify-machineinstrs to every llc invocation? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68341