[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 accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

LGTM. The patch is easy to follow now. Thank you for your patience.



================
Comment at: llvm/lib/Target/PowerPC/PPCMCInstLower.cpp:34
                                       AsmPrinter &AP) {
-  const TargetMachine &TM = AP.TM;
-  Mangler &Mang = TM.getObjFileLowering()->getMangler();
-  const DataLayout &DL = AP.getDataLayout();
-  MCContext &Ctx = AP.OutContext;
-
-  SmallString<128> Name;
-  if (!MO.isGlobal()) {
-    assert(MO.isSymbol() && "Isn't a symbol reference");
-    Mangler::getNameWithPrefix(Name, MO.getSymbolName(), DL);
-  } else {
+  if (MO.isGlobal()) {
     const GlobalValue *GV = MO.getGlobal();
----------------
Please add a comment here as to why this method is preferred. Perhaps something like:
```
// Get the symbol from the global, accounting for XCOFF-specific
// intricacies (see TargetLoweringObjectFileXCOFF::getTargetSymbol).
```


================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-nonfunc-calls.ll:40
 ; CHECK-DAG: addi [[REG3:[0-9]+]], [[REG1]], tls_something at tprel@l
-; CHECK-DAG: ld [[REG2:[0-9]+]], 0([[REG3]])
+; CHECK-DAG: ld [[REG2:[0-9]+]], tls_something at tprel@l([[REG1]])
 ; CHECK-DAG: ld 11, 16([[REG3]])
----------------
amyk wrote:
> @nemanjai I think remember before that you mentioned that perhaps it is not necessary to check if the `Subtarget.hasAIXSmallLocalExecTLS();` flag in `PPCISelDAGToDAG.cpp` - and that checking if the global variable is aligned should be sufficient.
> 
> Through the recent comments on this patch, I think it makes sense that we don’t need to check for this flag.  This would mean the change that I am doing apply to Linux, as well. Furthermore, I also updated `PPCISelDAGToDAG.cpp` to remove a portion that seemed like it had an equivalent portion later on in the file.
> 
> After doing some functional testing, I didn’t see any issues, with the exception of one Linux test case affected by the change I am making. This test case was introduced awhile back in `87deb0b8e3f14855e6c4652993be86684c2dd429`. I believe in here we’re bitcasting a TLS global and we’re treating this as a call through a function pointer. The node we’re dealing with in the test case is a `GlobalAddressSDNode` and it is aligned/passes my alignment check, so it ends up getting folded.
> 
> Before my patch, the TLS relocation is on the addis/addi as expected, and the subsequent loads load off of r3:
> ```
> # %bb.0:                                # %entry
> 	mflr 0
> 	stdu 1, -112(1)
> 	addis 3, 13, tls_something at tprel@ha
> 	std 0, 128(1)
> 	addi 3, 3, tls_something at tprel@l
> 	std 2, 40(1)
> 	ld 4, 0(3)
> 	ld 11, 16(3)
> 	ld 2, 8(3)
> 	mtctr 4
> 	bctrl
> 	ld 2, 40(1)
> 	addi 1, 1, 112
> 	ld 0, 16(1)
> 	mtlr 0
> 	blr
> ```
> And after my patch,  the ld with the zero offset gets affected, and none of the others do (which makes sense because we have non-zero offsets).
> ```
> # %bb.0:                                # %entry
> 	mflr 0
> 	stdu 1, -112(1)
> 	addis 3, 13, tls_something at tprel@ha
> 	std 0, 128(1)
> 	addi 4, 3, tls_something at tprel@l
> 	ld 3, tls_something at tprel@l(3)
> 	std 2, 40(1)
> 	ld 11, 16(4)
> 	ld 2, 8(4)
> 	mtctr 3
> 	bctrl
> 	ld 2, 40(1)
> 	addi 1, 1, 112
> 	ld 0, 16(1)
> 	mtlr 0
> 	blr
> ```
> In any case, I figured I’d ask if you have any thoughts/opinions on this since you requested changes and also had the original thought of not checking for `hasAIXSmallLocalExecTLS()` in the beginning.
This seems fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155600