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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 10 inline comments as done.
Xiangling_L added inline comments.


================
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),
----------------
jasonliu wrote:
> 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?
Some of my rational of adding these two properties are as follows, and please feel free to raise your further concerns:

1. For the instruction property `hasSideEffects`, it's used to indicate "does the instruction have side effects that are not captured by any operands of the instruction or other flags". And it's unset by default, the TableGen will infer its value from the  instruction pattern when possible. One rationale I used to add this property is that since `ADDIStocHA` and `ADDIStocHA8` [using under 64-bit mode] have the same instruction pattern, it should be safe to explicitly specify this `hasSideEffects=0` for `ADDIStocHA`.  And also considering `ADDIStocHA` itself, what it does is to load a value from TOC, it does have no side effect like implicit function call, volatile variable read etc, so I set this property to be 0.

2.For the instruction property `isReMaterializable = 1`, meaning this instruction has no side effects and requires no operands that aren't always available. The only allowed uses are constants and unallocatable physical registers so that the instructions result is independent of the place in the function. 
Since it loads value from TOC, and TOC register is always reserved on PPC target, so I think it's safe to set it as 1. Plus, the same reason as above, I would prefer sync the behavior of `ADDIStocHA` and ADDIStocHA8`. 




================
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
+
----------------
jasonliu wrote:
> Do we want to add -verify-machineinstrs to every llc invocation? 
Yes, I will add it and cpu level for testcases as well.


================
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
+
----------------
sfertile wrote:
> Please add '-verify-machine-instr` and an mcpu  option to each llc invocation. 
Thank you for reminding me of this, will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68341