[AIX] Support per global code model. #79202

mandlebug · 2024-01-23T20:14:04Z

Exploit the per global code model attribute on AIX. On AIX we need to update both the code sequence used to access the global (either 1 or 2 instructions for small and large code model respectively) and the storage mapping class that we emit the toc entry.

diggerlin · 2024-01-24T15:11:29Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+    return ModuleModel;
+
+  std::optional<CodeModel::Model> MaybeCodeModel =
+      dyn_cast<GlobalVariable>(GV)->getCodeModel();


dyn_cast maybe return a nullptr, do we need to check dyn_cast(GV) not nullptr here ?

We have an early return on !isa<GlobalVariable>(GV)) so we know dyn_cast must not fail - and because of that I should be using cast<> instead. I'll update it.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

diggerlin · 2024-01-29T18:28:41Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+    return;
+
+  const GlobalVariable *GVar = cast<GlobalVariable>(GV);
+  std::optional<CodeModel::Model> MaybeCM = GVar->getCodeModel();


nit: do not need to introduce a new variable GVar, since it only be used once

diggerlin · 2024-01-29T19:20:48Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+  case MachineOperand::MO_GlobalAddress: {
+    const GlobalValue *GV = MO.getGlobal();
+    MCSymbol *Sym = AP.getSymbol(GV);
+    checkPerGlobalCodeModel(GV, Sym);


I am not sure whether it is a good place to checkPerGlobalCodeModel in the getMCSymbolForTOCPseudoMO , This means that an MCSymbolXCOFF may end up setting PerSymbolCodeModel several times, depending on how many times the GV is used as an operand

how about to do it in

bool PPCAIXAsmPrinter::doInitiation() { ..... for (const auto &G : M.globals()) { .. MCSymbol *Sym = getSymbol(&G); checkPerGlobalCodeModel(%G, Sym) }

llvm/test/CodeGen/PowerPC/aix-codemodel-attr.ll

diggerlin · 2024-02-21T20:39:49Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

-    return;
-
+static std::optional<CodeModel::Model>
+hasPerGlobalCodeModel(const GlobalValue *GV) {


nit , the functionality looks like to get the CodeModel::Model , change the function name to getPerGlobalCodeModel

diggerlin · 2024-02-21T20:51:33Z

llvm/test/CodeGen/PowerPC/aix-codemodel-attr.ll

+; CHECK-SMALL32: lwz [[SCRATCH:[0-9]+]], L..C[[TL_F:[0-9]+]](2)
+; CHECK-LARGE64: ld [[SCRATCH:[0-9]+]], L..C[[TL_F]]@l([[HI]])
+; CHECK-SMALL64: ld  [[SCRATCH:[0-9]+]], L..C[[TL_F:[0-9]+]](2)
+; CHECK: lwz 3, 0([[SCRATCH]])


since the only different of 32bit and 64 bit is lwz and ld
for example:
CHECK-LARGE32: lwz [[SCRATCH:[0-9]+]], L..C[[TL_F]]@l([[HI]])
CHECK-LARGE64: ld [[SCRATCH:[0-9]+]], L..C[[TL_F]]@l([[HI]])

I will change like as following to make the test case simple.

RUN: %s | FileCheck --check-prefixes=CHECK,CHECK32,CHECK-LARGE,CHECK-LARGE32

to

RUN: %s | FileCheck -DINSTR=lwz --check-prefixes=CHECK,CHECK32,CHECK-LARGE

CHECK-LARGE: [[INSTR]] [[SCRATCH:[0-9]+]], L..C[[TL_F]]@l([[HI]]

I like the suggestion in that it simplifies the number of check prefixes we have, but then it also buries some of the details behind the macro. I was looking at the difference in the toc peephole patch (since the testing there seems to have gone from using the macro to now using it) and personally prefer having the extra check but easier to read output - mainly because we are already impacting the readability by having to have match variables for the labels in the TOC. If you feel really strongly let me know and I will change it over to your suggestion.

diggerlin · 2024-02-21T20:53:04Z

llvm/test/CodeGen/PowerPC/aix-codemodel-attr.ll

+}
+; CHECK:   addis [[HI:[0-9]+]], L..C[[TL_D]]@u(2)
+; CHECK32: lwz 3, L..C[[TL_D]]@l([[HI]])
+; CHECK64: ld 3, L..C[[TL_D]]@l([[HI]])


ditto, we can use -DINSTR=lwz or -DINSTR=ld

diggerlin · 2024-02-21T20:54:00Z

llvm/test/CodeGen/PowerPC/aix-codemodel-attr.ll

+; CHECK-LARGE32: lwz [[SCRATCH:[0-9]+]], L..C[[TL_E]]@l([[HI]])
+; CHECK-SMALL32: lwz [[SCRATCH:[0-9]+]], L..C[[TL_E:[0-9]+]](2)
+; CHECK-LARGE64: ld  [[SCRATCH:[0-9]+]], L..C[[TL_E]]@l([[HI]])
+; CHECK-SMALL64: ld  [[SCRATCH:[0-9]+]], L..C[[TL_E:[0-9]+]](2)


ditto as below comment.

syzaara · 2024-02-21T20:58:54Z

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

+                                            const TargetMachine &TM) {
+    const MCSymbolXCOFF *XSym = cast<MCSymbolXCOFF>(Sym);
+    // Use large code model toc entries for ehinfo symbols as they are
+    // never refrenced directly. The runtime loads their TOC entries


nit: refrenced -> referenced

syzaara · 2024-02-21T21:00:01Z

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

+                                            const TargetMachine &TM) {
+    const MCSymbolXCOFF *XSym = cast<MCSymbolXCOFF>(Sym);
+    // Use large code model toc entries for ehinfo symbols as they are
+    // never refrenced directly. The runtime loads their TOC entries


nit: their TOC entries address -> their TOC entry addresses

syzaara · 2024-02-21T21:01:43Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+                                     const TargetMachine &TM,
+                                     const MachineOperand &MO) {
+  CodeModel::Model ModuleModel = TM.getCodeModel();
+  // Per global code model is only support on AIX.


nit: only support -> only supported

syzaara · 2024-02-21T21:17:08Z

llvm/lib/Target/PowerPC/PPCSubtarget.cpp

@@ -176,6 +176,15 @@ bool PPCSubtarget::isGVIndirectSymbol(const GlobalValue *GV) const {
  // Large code model always uses the TOC even for local symbols.
  if (TM.getCodeModel() == CodeModel::Large)


What happens if the TM.getCodeModel is large, but the specific GV is set to small?

This was a really good question. I think originally I added this because it was triggering an assertion in the PPCAsmPrinter when we would emit the large code model nodes when the module code model was small. In reality both small and large code model use an indirect. That is true for both XCOFF and ELF - ELF has medium code model where we accessing things toc-relative instead of got indirect. I changed the AIX code to check for the toc-data attribute, since that is the only time a symbol would not be indirected on AIX.

The existing code is slightly wrong in that we should be returning true for both small and large code model, but because of where (and how) this function is used that change will be untestable. We can update it in a separate NFC patch to make it correct at least, and when we add support for per global code model for ELF then it would be come testable.

mandlebug · 2024-03-04T20:59:12Z

rebased and force pushed as there were conflicting changes that had landed since this PR was first posted.

github-actions · 2024-03-04T21:00:58Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm/include/llvm/MC/MCSymbolXCOFF.h

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

amy-kwan · 2024-03-06T16:07:58Z

llvm/lib/Target/PowerPC/PPCSubtarget.cpp

@@ -186,9 +186,18 @@ bool PPCSubtarget::enableSubRegLiveness() const {
 }

 bool PPCSubtarget::isGVIndirectSymbol(const GlobalValue *GV) const {
+  if (isAIXABI() && isa<GlobalVariable>(GV)) {
+    // On AIX the only symbols that aren't indirect are toc-data.
+    if (cast<GlobalVariable>(GV)->hasAttribute("toc-data"))


nit: According to https://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates, we should try not to use isa<> followed by <cast>?

updated to use dyn_cast

llvm/lib/Target/PowerPC/PPCSubtarget.cpp

chenzheng1030

At first glance, the code diff is larger than what I thought. I original thought we just need a small change to fix the code generation for per global code model in ISEL-DAG phase.

But seems to keep the optimization of keeping large code model symbols with TE SMC, most of the code changes are for recording code model for a XCOFF symbol because there is no direct mapping between MCSymbolXCOFF and GlobalVariable. I cannot find a better way to avoid these codes either...

chenzheng1030 · 2024-03-11T02:29:54Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

-  case MachineOperand::MO_GlobalAddress:
-    return AP.getSymbol(MO.getGlobal());
+  case MachineOperand::MO_GlobalAddress: {
+    const GlobalValue *GV = MO.getGlobal();


This seems unnecessary?

llvm/test/CodeGen/PowerPC/aix-codemodel-attr.ll

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

diggerlin

I put other two small comment. I do not have further comment on the patch, please address zhengchen's comment and wait for zhengchen happy with the PR.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

diggerlin · 2024-03-11T18:07:10Z

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

+    return CM;
+  }
+
+  return ModuleModel;


nit : we can change the code from line 574 ~591 to (I do not have strong suggestion on the change, feel free to keep your code if you want)

if (auto *GValue = dyn_cast<GlobalAddressSDNode>(Node->getOperand(0)){ if(auto *GVar = dyn_cast<GlobalVariable>(GV)){ std::optional<CodeModel::Model> MaybeCodeModel =GV->getCodeModel(); if (MaybeCodeModel) { CodeModel::Model CM = *MaybeCodeModel; assert((CM == CodeModel::Small || CM == CodeModel::Large) && "invalid code model for AIX"); return CM; } } } return ModuleModel;

llvm/lib/Target/PowerPC/PPCSubtarget.cpp

chenzheng1030

LGTM too. Thanks for making the improvement.

Exploit the per global code model attribute on AIX. On AIX we need to update both the code sequence used to access the global (either 1 or 2 instructions for small and large code model respectively) and the storage mapping class that we emit the toc entry.

- convert assert to early return. - change dyn_cast<> following an isa<> to a cast<> - remove a null check after a cast<>

- Change setting per global code model on MCSymbol to the ASMPrinters initialization so it happens once. - Add some globals without explicit code models to the lit test.

- Fixed some spelling mistakes in comments. - renamed function that retreives the per global code model. - updated the AIX specific code in isGVIndirectSymbol.

Co-authored-by: Amy Kwan <akwan0907@gmail.com>

- use dyn_cast instad of isa + cast - remove erroneously added newline in otherwise untouched code.

- Added support for looking through aliases to the aliasees objects code model, along with lit testing. - Split the shared code of 'getCodeModel' into PPCSubtarget and have PPCISelDAGToDAG and PPCAsmPrinter use that. - Undo unintended change to 'getMCSymbolForTOCPsudoMO' that was left when addressing earlier comments.

mandlebug added backend:PowerPC platform:aix labels Jan 23, 2024

mandlebug requested review from chenzheng1030, maryammo, diggerlin, stefanp-ibm and syzaara January 23, 2024 20:14

mandlebug self-assigned this Jan 23, 2024

diggerlin reviewed Jan 24, 2024

View reviewed changes

diggerlin reviewed Jan 29, 2024

View reviewed changes

diggerlin reviewed Jan 30, 2024

View reviewed changes

llvm/test/CodeGen/PowerPC/aix-codemodel-attr.ll Show resolved Hide resolved

diggerlin reviewed Feb 21, 2024

View reviewed changes

syzaara reviewed Feb 21, 2024

View reviewed changes

mandlebug force-pushed the sfertile/AIXCodeModelAttribute branch from 3dbf6c9 to 9e5eb9a Compare March 4, 2024 20:58

amy-kwan reviewed Mar 6, 2024

View reviewed changes

chenzheng1030 reviewed Mar 11, 2024

View reviewed changes

diggerlin approved these changes Mar 11, 2024

View reviewed changes

chenzheng1030 reviewed Mar 13, 2024

View reviewed changes

llvm/lib/Target/PowerPC/PPCSubtarget.cpp Show resolved Hide resolved

chenzheng1030 approved these changes Mar 14, 2024

View reviewed changes

mandlebug force-pushed the sfertile/AIXCodeModelAttribute branch from 3fd3217 to 701e485 Compare March 14, 2024 16:22

mandlebug and others added 6 commits March 15, 2024 10:19

Address first round of review comments.

f9be21b

- convert assert to early return. - change dyn_cast<> following an isa<> to a cast<> - remove a null check after a cast<>

Address second round of review comments.

8207b6a

- Change setting per global code model on MCSymbol to the ASMPrinters initialization so it happens once. - Add some globals without explicit code models to the lit test.

Address third reound of review comments.

eca388d

- Fixed some spelling mistakes in comments. - renamed function that retreives the per global code model. - updated the AIX specific code in isGVIndirectSymbol.

Fix formating.

8e712ec

Update llvm/include/llvm/MC/MCSymbolXCOFF.h

eb09fd5

Co-authored-by: Amy Kwan <akwan0907@gmail.com>

mandlebug and others added 3 commits March 15, 2024 10:19

Update llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

980ae19

Co-authored-by: Amy Kwan <akwan0907@gmail.com>

Minor format/style changes to PPCSubtarget::isGVIndirectSymbol

39c7802

- use dyn_cast instad of isa + cast - remove erroneously added newline in otherwise untouched code.

mandlebug force-pushed the sfertile/AIXCodeModelAttribute branch from 701e485 to a77ce54 Compare March 15, 2024 15:15

lei137 merged commit 2d80505 into llvm:main Mar 15, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIX] Support per global code model. #79202

[AIX] Support per global code model. #79202

mandlebug commented Jan 23, 2024

diggerlin Jan 24, 2024

mandlebug Jan 25, 2024

diggerlin Jan 29, 2024

diggerlin Jan 29, 2024 •

edited

diggerlin Feb 21, 2024

diggerlin Feb 21, 2024 •

edited

mandlebug Mar 4, 2024

diggerlin Feb 21, 2024 •

edited

diggerlin Feb 21, 2024

syzaara Feb 21, 2024

syzaara Feb 21, 2024

syzaara Feb 21, 2024

syzaara Feb 21, 2024

mandlebug Feb 28, 2024

mandlebug commented Mar 4, 2024

github-actions bot commented Mar 4, 2024 •

edited

amy-kwan Mar 6, 2024

mandlebug Mar 7, 2024

chenzheng1030 left a comment

chenzheng1030 Mar 11, 2024

diggerlin left a comment

diggerlin Mar 11, 2024 •

edited

chenzheng1030 left a comment

		@@ -176,6 +176,15 @@ bool PPCSubtarget::isGVIndirectSymbol(const GlobalValue *GV) const {
		// Large code model always uses the TOC even for local symbols.
		if (TM.getCodeModel() == CodeModel::Large)

[AIX] Support per global code model. #79202

[AIX] Support per global code model. #79202

Conversation

mandlebug commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Jan 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandlebug commented Mar 4, 2024

github-actions bot commented Mar 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenzheng1030 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin left a comment

Choose a reason for hiding this comment

diggerlin Mar 11, 2024 • edited

Choose a reason for hiding this comment

chenzheng1030 left a comment

Choose a reason for hiding this comment

diggerlin Jan 29, 2024 •

edited

diggerlin Feb 21, 2024 •

edited

diggerlin Feb 21, 2024 •

edited

github-actions bot commented Mar 4, 2024 •

edited

diggerlin Mar 11, 2024 •

edited