[PPC64LE] poly_tomont: trim dead callee-saved spills (frame 320->224)

[Scottcjn]

Addresses the inline TODO in poly_tomont_ppc_asm.S (#1722).

The prologue saved and restored v20-v30, but the body only clobbers
v23, v24, and v27-v30. v20, v21, v22, v25, and v26 were spilled and
reloaded but never used: five dead stxvx/lxvx pairs and 80 bytes of
frame. This saves only the registers the body touches and trims the
frame from 320 to 224 bytes. The Montgomery multiply is unchanged.

Validation: func, kat, and unit all pass for ML-KEM-512/768/1024
(opt and no_opt) on a POWER8 S824 with -DMLK_FORCE_PPC64LE -mcpu=power8.
The mlkem/ copy was regenerated with scripts/simpasm.

Supersedes #1752, cleaned up per review feedback. Closes #1722.

[mkannwischer]

Thanks @Scottcjn. Below some nits. Could you address them first, and then we'll get the code changes reviewed.

[mkannwischer]

Why was this changed? Please change it back.

[mkannwischer]

That's the old PR number. I would prefer not linking specific PRs.
We don't usually add names to files. Is it important to keep your name in here?

[mkannwischer]

If you insist, you can add a copyright line higher in the file

Copyright (c) Scott Boudreaux

[Scottcjn]

Thanks @mkannwischer. Nits addressed:

• Dropped the attribution line from both files.
• Restored the auto-generated SCU comment in the dev source.
• Regenerated the mlkem/ copy so it is back to the byte-for-byte simpasm output.

The assembly is unchanged from the reviewed version, so what is left vs main is just the frame trim (320 to 224) and the matching save/restore set. Ready for the code review whenever you are.

[mkannwischer]

Thanks for your updates. The last 3 commits are missing the DCO signoff.
Also, could you please squash your 4 commits into one?

[Scottcjn]

Done. Squashed to a single commit with the DCO Signed-off-by line. Thanks.

[mkannwischer]

Thanks @Scottcjn.

@bhess, @dannytsen, could either if you provide a review for this change please?

[dannytsen]

@mkannwischer The change looks OK to me. A note, the code we submitted is copyrighted under IBM and no other names can be added in the code unless a complete re-write of the code. So any modification is still under IBM copyright and owned by IBM. I have to make it clear.

[hanno-becker]

... and no other names can be added in the code unless a complete re-write of the code.

I don't think this is true? The code is licensed under MIT / Apache / ISC.

Of course, IBM's copyright notice would be retained when creating a derivative.

[Scottcjn]

Thanks both. The review comments are all addressed in the current head:

• Restored the Don't modify by hand -- this is auto-generated by scripts/autogen line in the SCU-undef comment block, so the notice is back to matching the other files.
• Dropped the specific PR-number reference.
• Removed the added name line from the header. The existing Copyright (c) IBM Corp. and Written by Danny Tsen attributions are unchanged.

The only functional change that remains is the frame trim itself: the epilogue now saves/restores just the callee-saved VRs the body actually clobbers (v23, v24, v27-v30), dropping the five unused v20-v22/v25/v26 spills and shrinking the frame from 320 to 224 bytes.

@hanno-becker on the licensing point, agreed, the IBM copyright notice stays in the header. If nothing else is outstanding, this should be ready for another look. Thanks for the reviews.

[mkannwischer]

Thanks @Scottcjn for your patience!
Thanks @dannytsen for reviewing this!

This looks good to me.

[mkannwischer]

Running full CI and benchmarks in #1799

[mkannwischer]

Hmm, the benchmarks on POWER10 are not looking good:
For ML-KEM-1024:

On main: mlk_polyvec_tomont cycles=439
This PR: mlk_polyvec_tomont cycles=535

Full benchmarks also show a small overall slowdown (see #1799 (review))

ppc64le (POWER10) benchmarks

Benchmark suite	Current: 2a7a6c0	Previous: cfd1912	Ratio
ML-KEM-512 keypair	38267 cycles	37977 cycles	1.01
ML-KEM-512 encaps	43846 cycles	43448 cycles	1.01
ML-KEM-512 decaps	54160 cycles	53423 cycles	1.01
ML-KEM-768 keypair	67792 cycles	67467 cycles	1.00
ML-KEM-768 encaps	76830 cycles	76412 cycles	1.01
ML-KEM-768 decaps	91045 cycles	90618 cycles	1.00
ML-KEM-1024 keypair	108949 cycles	108562 cycles	1.00
ML-KEM-1024 encaps	119891 cycles	118622 cycles	1.01
ML-KEM-1024 decaps	137980 cycles	137281 cycles	1.01

@Scottcjn, any idea why this results in an overall slowdown on POWER10?

benchmarks show no improvements on POWER10; we first need to discuss that.

[Scottcjn]

Thanks for running the numbers, and sorry for the slow reply.

I don't have POWER10 hardware, so I can't reproduce the 439 vs 535 directly and I won't guess at numbers I can't measure. Here is what I can measure and what I think is going on.

On a POWER8 S824 (Ubuntu 20.04, gcc 9.4, make bench_components_1024 CYCLES=PERF OPT=1, taskset to one core, 10 runs of the median-of-20 harness) the trim is a clear win. On current main, ML-KEM-1024 mlk_poly_tomont_native is 105 cycles and mlk_polyvec_tomont is 444 to 446. With this PR it is 99 to 100 and 409 to 412. The linker happened to place poly_tomont_ppc_asm at the same address in both builds, so that comparison is not polluted by code layout.

On the why: poly_tomont_ppc_asm is straight-line code, no loop, and the patch strictly removes instructions (10 in the prologue, 12 in the epilogue) without touching the arithmetic. So my best explanation for the P10 delta is code placement rather than the trim itself. The prologue shrink moves the body start by 40 bytes, and since the simpasm-generated file only carries .balign 4, the function's address in the linked bench binary is whatever the link order gives it. I tested placement sensitivity on the POWER8 by padding the entry to different offsets mod 64: mlk_polyvec_tomont swings between 410 and 423 cycles from placement alone, and pinning the entry to a 64 byte boundary was actually one of the slower placements there. If the P10 front end is more sensitive to where this block lands, that could account for a good chunk of 439 to 535, but I can't verify that without the hardware, so I'm not adding any alignment directive on a guess.

I have rebased the branch onto current main (3853864). The diff is unchanged, still just the frame trim, single signed-off commit. func, kat and unit tests pass on the POWER8 for all three parameter sets. Since the label namespacing change reshuffled layout anyway, could you re-run the P10 bench on the new head when you get a chance? If mlk_polyvec_tomont still lands around 535, it would be worth comparing the address of poly_tomont_ppc_asm mod 64 in the two bench binaries. And if it turns out P10 genuinely pays for the smaller frame, I'm fine with dropping this; a 96 cycle regression on P10 is not worth 6 cycles per call on P8. Longer term, having simpasm emit a larger .balign would make the ppc64le numbers less layout dependent, but that changes every generated file across backends, so that would be your call and a separate PR if wanted.