[AIE2PS] Fix alignment of 64-bit spills and improve CSR spills of L registers by khallouh · Pull Request #1001 · Xilinx/llvm-aie

khallouh · 2026-05-13T16:09:04Z

No description provided.

niwinanto

LGTM!

andcarminati · 2026-05-18T07:50:50Z

+; CHECK-NEXT:    lda m2, [p6], #-4; mov dj3, #0
+; CHECK-NEXT:    lda dj2, [p6], #-4; mov dc2, dj4
+; CHECK-NEXT:    lda dj6, [p6], #-4; movs dc6, dj4; or r30, r5, r5; mov r5, dj4
+; CHECK-NEXT:    lda dn2, [p6, #0]; st r9:r8, [sp, #-64]; or r8, r7, r7; mov r7, dj4 // 8-byte Folded Spill


For some reason we are spilling more. luckily we could pack the stores within available bundles and we also have more unused stack because of the alignment (64 bytes). Do you have an idea?

yes, see comment above

khallouh · 2026-05-18T13:08:16Z

Updated. There are no regressions anymore including in mllib L1 (full run). Also conv2 and gemm don't change in the tests. The improvements in those kernels are mostly in the setup and main functions. I am currently running mllib again (reduced run) to get updated improvement numbers since they are supposed to increase with the last commit.

khallouh · 2026-05-18T16:33:59Z

There are only improvements, zero regressions

niwinanto · 2026-05-19T08:06:12Z

+    // Determine if both subregisters actually need saving.
+    // LRegMarked alone doesn't mean both - check individual GPR marks.
+    bool BothNeeded =
+        (EvenMarked && OddMarked) || (LRegMarked && !EvenMarked && !OddMarked);


Do we have both L and R at the same time? Can we assert?

The check !EvenMarked && !OddMarked is meant for the case where only one GPR is touched, since we have its super-register in the CSR list it will also be marked by the generic determineCalleeSaves for saving, but we don't want to save the entire L just the gpr. This is then handled in the else case where we reset the L and mark the single GPR that is actually needed.

} else { // Only one GPR needs saving - clear L and keep only the needed GPR. SavedRegs.reset(LReg); if (EvenMarked || (LRegMarked && !OddMarked)) SavedRegs.set(EvenGPR); else SavedRegs.reset(EvenGPR); if (OddMarked || (LRegMarked && !EvenMarked)) SavedRegs.set(OddGPR); else SavedRegs.reset(OddGPR); }

It means, there could be cases L register and corresponding R register marked at the same time, right?

Exactly, but only one of them. This is a big limitation of the generic determineCalleeSaves as it will happily mark all super registers for saving even if only of their subregisters is actually used. So far this has not been an issue as our CSR list only contained atomic registers.

Nit: You could add an assert assert((!EvenMarked || LRegMarked) && (!OddMarked || LRegMarked) && "sub-reg mark without L pair mark violates invariant");

niwinanto · 2026-05-19T08:30:54Z

+  static const MCPhysReg LRegs[] = {AIE2PS::l4, AIE2PS::l5, AIE2PS::l6,
+                                    AIE2PS::l7};


Suggested change

static const MCPhysReg LRegs[] = {AIE2PS::l4, AIE2PS::l5, AIE2PS::l6,

AIE2PS::l7};

const MCPhysReg *CSRegs = MF.getRegInfo().getCalleeSavedRegs();

for (unsigned i = 0; CSRegs[i]; ++i) {

MCPhysReg Reg = CSRegs[i];

if (!AIE2PS::eLRegClass.contains(Reg))

continue;

...

Since we use multiple calling conventions, would be nice to derive this list than hardcoding. At this moment it has no correctness impact, but it can change in the future.

It is a hardcoded list anyway as part of the ABI. I don't think it will change for this target, this is already in an aie2ps target specific hook. We won't be sharing this with another target. If the same list is needed somewhere else in the future, we can factor it out for reuse.

Update: The list should not change for aie2ps but we have a test save_partial_L that overrides the calee saved list to exclude $l4. Once I added the assert below, it triggered it

assert((!EvenMarked || LRegMarked) && (!OddMarked || LRegMarked) && "sub-reg mark without L pair mark violates invariant");

So I will take your suggestion to derive the list

niwinanto · 2026-05-19T08:57:10Z

+
+    if (BothNeeded) {
+      // Both subregisters need saving.
+      if (MFI.hasCalls()) {


I don`t fully understand why we have to do special handling with a call in the function, since we are working on callee saved registers. Would be nice to include a comment for future refernce.

As discussed offline, when there is calls we mark the L register so that we get a single spill instead of 2. When there are no calls, we prefer marking the subregsiters since they can be copied to non CSR registers instead of spilled to memory (There is no move instruction between L registers). For the call case we have no choice but to spill anyway since we don't know which registers the callee is going to use. I'll add this as a comment.

khallouh · 2026-05-20T17:57:51Z

I addressed the comments and fixed up the test for the call case to reflect realistic MIR (not using CSR registers in the PseudoRET)

l registers are only considered callee-saved by the prologepilog emitter via their subregisters, so when a greedy allocates an l register that is callee-saved, the prologeemitter will save each of its gpr subregisters instead of saving the entire l directly. This adds those l registers to the csr_aie2ps list

andcarminati · 2026-05-21T07:24:42Z

This looks good, thank you for fixing this issue! I I just left some comments.

niwinanto

LGTM! Thanks for addressing the comments.

…r members is used

The test was using CSR registers for function returns which led to the reload happening before the PseudoRET where it would have to be afterwards for a normal use. We ended up returning the l register that was saved from the caller function instead of the one defined inside the function. This fixes the test to show realistic MIR where the register is used by a KILL instruction instead of the PseudoRET, now the reload happens after the KILL and right before the return restoring the CSR value coming from the caller.

khallouh requested review from F-Stuckmann, SagarMaheshwari99, abhinay-anubola, abnikant, andcarminati, katerynamuts, konstantinschwarz, martien-de-jong, mludevid, niwinanto and stephenneuendorffer as code owners May 13, 2026 16:09

martien-de-jong reviewed May 15, 2026

View reviewed changes

Comment thread llvm/lib/Target/AIE/aie2ps/AIE2PSInstrInfo.cpp Outdated

khallouh force-pushed the khallouh.fix.64.align branch from c5e3584 to eb7dacd Compare May 15, 2026 12:54

niwinanto previously approved these changes May 15, 2026

View reviewed changes

khallouh dismissed niwinanto’s stale review via 00e3cfd May 15, 2026 13:36

khallouh force-pushed the khallouh.fix.64.align branch 2 times, most recently from 00e3cfd to f514ecf Compare May 15, 2026 14:06

niwinanto reviewed May 15, 2026

View reviewed changes

Comment thread llvm/test/CodeGen/AIE/aie2ps/bfp16_13_mul.ll Outdated

andcarminati reviewed May 18, 2026

View reviewed changes

khallouh force-pushed the khallouh.fix.64.align branch from f514ecf to 63e04b5 Compare May 18, 2026 12:29

khallouh changed the title ~~[AIE2PS] Fix alignment of 64-bit spills~~ [AIE2PS] Fix alignment of 64-bit spills and improve CSR spills of L registers May 18, 2026

niwinanto reviewed May 19, 2026

View reviewed changes

khallouh force-pushed the khallouh.fix.64.align branch from 63e04b5 to 51e5ec7 Compare May 20, 2026 17:55

khallouh added 3 commits May 20, 2026 11:58

[AIE2PS] Add l register spill-reload test

5107a0e

[AIE2PS] Fix alignment of 64-bit spills

5bcacc0

khallouh force-pushed the khallouh.fix.64.align branch from 51e5ec7 to 7feb5da Compare May 20, 2026 18:07

andcarminati reviewed May 21, 2026

View reviewed changes

Comment thread llvm/lib/Target/AIE/aie2ps/AIE2PSFrameLowering.cpp Outdated

andcarminati reviewed May 21, 2026

View reviewed changes

Comment thread llvm/lib/Target/AIE/aie2ps/AIE2PSFrameLowering.cpp Outdated

andcarminati reviewed May 21, 2026

View reviewed changes

Comment thread llvm/lib/Target/AIE/aie2ps/AIE2PSFrameLowering.cpp

khallouh force-pushed the khallouh.fix.64.align branch from 7feb5da to 8e0f01c Compare May 21, 2026 09:28

niwinanto reviewed May 21, 2026

View reviewed changes

Comment thread llvm/lib/Target/AIE/aie2ps/AIE2PSFrameLowering.cpp Outdated

khallouh force-pushed the khallouh.fix.64.align branch from 8e0f01c to 0923f24 Compare May 21, 2026 10:16

niwinanto previously approved these changes May 21, 2026

View reviewed changes

khallouh dismissed niwinanto’s stale review via 220b48e May 21, 2026 10:32

khallouh force-pushed the khallouh.fix.64.align branch from 0923f24 to 220b48e Compare May 21, 2026 10:32

niwinanto reviewed May 21, 2026

View reviewed changes

Comment thread llvm/lib/Target/AIE/aie2ps/AIE2PSFrameLowering.cpp Outdated

khallouh added 2 commits May 21, 2026 04:43

[AIE2PS] Save CSR L registers as individual GPRs when only of the pai…

20545cc

…r members is used

khallouh force-pushed the khallouh.fix.64.align branch from 220b48e to cac0f5e Compare May 21, 2026 10:43

niwinanto approved these changes May 21, 2026

View reviewed changes

khallouh enabled auto-merge (rebase) May 21, 2026 10:46

khallouh merged commit b6cddea into aie-public May 21, 2026
7 checks passed

khallouh deleted the khallouh.fix.64.align branch May 21, 2026 11:29

		static const MCPhysReg LRegs[] = {AIE2PS::l4, AIE2PS::l5, AIE2PS::l6,
		AIE2PS::l7};

-  static const MCPhysReg LRegs[] = {AIE2PS::l4, AIE2PS::l5, AIE2PS::l6,
-                                    AIE2PS::l7};
+const MCPhysReg *CSRegs = MF.getRegInfo().getCalleeSavedRegs();
+for (unsigned i = 0; CSRegs[i]; ++i) {
+  MCPhysReg Reg = CSRegs[i];
+  if (!AIE2PS::eLRegClass.contains(Reg))
+    continue;
+  ...

Conversation

khallouh commented May 13, 2026

Uh oh!

Uh oh!

niwinanto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khallouh commented May 18, 2026

Uh oh!

khallouh commented May 18, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

khallouh commented May 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andcarminati commented May 21, 2026

Uh oh!

Uh oh!

niwinanto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants