[PATCH] D155600: [AIX][TLS] Produce a faster local-exec access sequence with -maix-small-local-exec-tls (And optimize when load/store offsets are 0)

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

Sorry, I forgot to submit these comments when I reviewed this.



================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1571
 
+bool PPCAsmPrinter::emitAIXSmallLocalExecTLSAccess(const MachineInstr *MI,
+                                                   MCInst &TmpInst) {
----------------
I'm not sure I follow what the purpose of this function is. It would seem that it checks whether we are lowering the AIX Small TLS instruction and change any `PPCIADDI8` to `PPC::LA8`. However, if neither of those are true, it doesn't do anything.

The issue I have with it is that if this returns `false`, we break out of the switch and simply call
```
LowerPPCMachineInstrToMCInst(MI, TmpInst, *this);
EmitToStreamer(*OutStreamer, TmpInst);
```
which would have happened if we never called this function anyway. So it would seem that we don't need the additions from lines 1541 to 1564. We can simply add `PPC::ADDI8` into the switch above, and change it to `PPC::LA8` if both of the following conditions are met:
`Subtarget->hasAIXSmallLocalExecTLS() && MO.getTargetFlags() & PPCII::MO_TPREL_FLAG`

Or maybe I'm missing something.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:7623
       // other cases provide aligned addresses and are always safe.)
-      if (RequiresMod4Offset &&
+      if (RequiresMod4Offset && !HasAIXSmallLocalExecTLS &&
           (!isa<ConstantSDNode>(Base.getOperand(1)) ||
----------------
hubert.reinterpretcast wrote:
> This change looks suspicious.
> 
> I am getting:
> ```
> Global must be word-aligned for LD, STD, LWA!
> UNREACHABLE executed at /terrannew/hstong/.Lpcoral03/llvmbld/dev/llvm-project/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1523!
> ```
> 
> With:
> ```
> clang --target=powerpc64-ibm-aix -maix-small-local-exec-tls -O -c -o /dev/null -xc - \
> <<<$'__attribute__((__tls_model__("local-exec"))) __thread struct { int x __attribute__((__packed__)); } a;\nint f() { return a.x; }'
> ```
Right, if `Base.getOperand(1)` is not a constant, we need to check the alignment of the `GlobalAddress`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3366
+      if (HasAIXSmallLocalExecTLS &&
+          (GVTypeSize < (AIXTLSUpperDisplacement - 8)))
+        return DAG.getNode(PPCISD::Lo, dl, PtrVT, VariableOffsetTGA, TLSReg);
----------------
hubert.reinterpretcast wrote:
> What does `AIXTLSUpperDisplacement` represent? It is already given a value of 32K - 8. Should the hard coded subtraction be here, should the value of `AIXTLSUpperDisplacement` be further adjusted instead, or is there unintentional double adjustment happening?
> 
> For reference, I encountered no issues linking the result of assembly that performs `la` (using the subject access pattern) for the past-the-end address of a 32K - 1 local-exec TLS variable.
I agree. We shouldn't be doing any arithmetic here on `AIXTLSUpperDisplacement`. We should set it to what we want it and only perform comparisons.


================
Comment at: llvm/lib/Target/PowerPC/PPCMCInstLower.cpp:59
+            .hasAIXSmallLocalExecTLS() &&
+        MO.getTargetFlags() & PPCII::MO_TPREL_FLAG) {
+      MCSymbol *Sym = AP.getSymbol(GV);
----------------
I suppose it doesn't matter for precedence of `&` over `&&`, but I think it is still easier to read this condition if you parenthesize this part as `(MO.getTargetFlags() & PPCII::MO_TPREL_FLAG)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155600