[AVR] Fix for issue #163015, occasional corruption in stack passed params #163082

carlos4242 · 2025-10-12T17:23:02Z

Corruption can occur with passing parameters on the stack when under register pressure.

See this forum discussion for details (or the GitHub issue) https://discourse.llvm.org/t/avr-register-allocation-issue-help-advice-with-debugging/88498/11

@aykevl @benshi001

benshi001 · 2025-10-13T10:40:10Z

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

+// stack adjustment then as a base pointer. To avoid corruption, we thus
+// specify special classes of registers, like GPR8 and DREGS, but with
+// the Z register removed, as the source/input to these instructions.
+// (Note: R29R28 are already reserved by the ABI so would never be used


(Note: R29R28 are already reserved by the ABI so would never be use ... is known, there is no need to mention that again. Other comment lines are OK.

benshi001 · 2025-10-13T10:54:02Z

llvm/test/CodeGen/AVR/dynalloca.ll


 define void @test1(i16 %x) {
 ; CHECK-LABEL: test1:
+


It would be better to split this PR into two commits.

The first one is a [NFC]: run update_test_checks.py against this test file, and the full assembly for this file would be supplemented.

Apply your functional changes, so we can see a clear contrast (before VS after) on this test file.

benshi001 · 2025-10-13T10:59:00Z

llvm/test/CodeGen/AVR/zreg-clobbering-bug.ll

@@ -0,0 +1,34 @@
+; RUN: llc < %s -mtriple=avr | FileCheck -v -dump-input always %s


It would be better to

rename the file to issue-163082.ll.

run update_llc_test_checks.py against this test without any handle modification.

benshi001

Please see my inline comments.

benshi001 · 2025-10-13T11:02:53Z

What's more, I appreciate if you can take a look at #104032, which seems have the same root cause. (However this can be another PR).

carlos4242 · 2025-10-14T23:57:37Z

@benshi001 cheers... those all sound like good suggestions, I'm a bit swamped with a new job this week. I'll try to do this quick tidy up for you on the weekend. Thank you so much for reviewing the PR. Hopefully we can get this in soon and get it out of the way.

With other issues, I'll try to take a look but it might be a while before I can commit time to working on other fixes, sadly. (I also have some possible conflicts of interests to resolve that might prevent me doing much llvm/AVR work even when I do get time, just to warn... but I'm sure at least passing on thoughts and diagnosing bugs for others to fix will be OK one day.)

carlos4242 · 2025-10-16T23:52:03Z

Was it meant to be update_llc_test_checks.py?

benshi001 · 2025-10-17T03:59:14Z

Was it meant to be update_llc_test_checks.py?

I am not sure how familiar you are with update_llc_test_checks.py, and I assume you never used it before.

Remove all AVR assembly lines in your .ll files, only keep the IR lines, and also keep the RUN: line.
Manually run llvm-project/utils/update_llc_test_checks.py with your test .ll files as a parameter, then you will get an updated .ll file with full assembly attached.

BTW: why you close this PR, I think this is a useful change and appreciate if it can be merged.

carlos4242 · 2025-10-17T18:31:50Z

Hiya @benshi001 ,

I tried renaming my branches and force reverted my main to the previous commit, which seemed to make GitHub automatically close the PR.

I'm still keen to get it in, I'm trying to wrap up and handover my AVR fixes as I'm stepping back from open source compiler work, and I also agree this is a good fix.

I think what you're saying about the comment makes sense and I've corrected it locally.

I hadn't used update_llc_test_checks.py before now but it was simple enough to work out. On one of your previous comments you mentioned update_test_checks.py, which I think is a different tool, useful for creating automated CHECK: lines if the tool being used is opt, instead of llc as here. So I just wanted to clarify that you probably meant that I should use update_llc_test_checks.py? In later comments you mention that tool. Since this is all CodeGen tests, it seems we should be talking about llc?

I understand the principles of what we're trying to do here, create tests that exactly track any change to the output, the problem is I think it makes more fragile and less accurate unit tests in this case.

The patch I created shouldn't really have made any difference to the dynalloca.ll test passing or failing, but the existing tests based on the output from the previous build was...

; CHECK: ldi r20, 0
; CHECK: ldi r21, 0
; CHECK: std Z+8, r21
; CHECK: std Z+7, r20
; CHECK: std Z+6, r21
; CHECK: std Z+5, r20
; CHECK: std Z+4, r21
; CHECK: std Z+3, r20
; CHECK: std Z+2, r21
; CHECK: std Z+1, r20

This is obviously a load into r20/r21 then store into various locations offset from Z. The fact that llc happened to pick r20/r21 is a bit arbitrary, it just finds a suitable pair of registers I think.

When my patch is applied and dynalloca.ll run again, only this test fails, and only in one place... it fails because the assembly is like...

ldi r18, 0
ldi r19, 0
std Z+8, r19
std Z+7, r19
...

...so by fairly random chance it happens to have decided to use r18/r19 instead of r20/r21, breaking the test, but I think that's arbitrary.

So I hand coded and changed the FileCheck lines to...

; CHECK: ldi [[REG1:r[0-9]+]], 0
; CHECK: ldi [[REG2:r[0-9]+]], 0
; CHECK: std Z+8, [[REG2]]
; CHECK: std Z+7, [[REG1]]
; CHECK: std Z+6, [[REG2]]
; CHECK: std Z+5, [[REG1]]
; CHECK: std Z+4, [[REG2]]
; CHECK: std Z+3, [[REG1]]
; CHECK: std Z+2, [[REG2]]
; CHECK: std Z+1, [[REG1]]

...which is logically the same but more resilient.

My concern is that if we use the autogenerated version, then patch and update it afterwards, you will see the diffs as you propose, but the differences are essentially non functional/trivial, and we will end up with a more fragile unit test, which can break again in the future for not very important reasons, creating false fails that need debugging.

Whereas my version that's hand corrected is more resilient than the original test, just as sensitive, and fixes the issue.

In the end, you're the maintainer so if you say "do it anyway" then I will, but I thought I'd flag the reason why I think a small tweak to a hand coded version is better here.

One thing I can definitely do is get rid of a lot of the other changes in the dynalloca.ll unit test and only leave the one change that's necessary to make the test pass if you'd like?

(I plan to do some work on dynamic allocation at some point in the future as I have private patches that I'd like to upstream one day but we can leave that separate.)

I'll put together another PR with an absolute bare minimum change, take a look and see if you can live with that.

Carl

benshi001 · 2025-10-18T04:02:12Z

Hiya @benshi001 ,

I tried renaming my branches and force reverted my main to the previous commit, which seemed to make GitHub automatically close the PR.

I'm still keen to get it in, I'm trying to wrap up and handover my AVR fixes as I'm stepping back from open source compiler work, and I also agree this is a good fix.

I think what you're saying about the comment makes sense and I've corrected it locally.

I hadn't used update_llc_test_checks.py before now but it was simple enough to work out. On one of your previous comments you mentioned update_test_checks.py, which I think is a different tool, useful for creating automated CHECK: lines if the tool being used is opt, instead of llc as here. So I just wanted to clarify that you probably meant that I should use update_llc_test_checks.py? In later comments you mention that tool. Since this is all CodeGen tests, it seems we should be talking about llc?

I understand the principles of what we're trying to do here, create tests that exactly track any change to the output, the problem is I think it makes more fragile and less accurate unit tests in this case.

The patch I created shouldn't really have made any difference to the dynalloca.ll test passing or failing, but the existing tests based on the output from the previous build was...
; CHECK: ldi r20, 0
; CHECK: ldi r21, 0
; CHECK: std Z+8, r21
; CHECK: std Z+7, r20
; CHECK: std Z+6, r21
; CHECK: std Z+5, r20
; CHECK: std Z+4, r21
; CHECK: std Z+3, r20
; CHECK: std Z+2, r21
; CHECK: std Z+1, r20
This is obviously a load into r20/r21 then store into various locations offset from Z. The fact that llc happened to pick r20/r21 is a bit arbitrary, it just finds a suitable pair of registers I think.

When my patch is applied and dynalloca.ll run again, only this test fails, and only in one place... it fails because the assembly is like...
ldi r18, 0
ldi r19, 0
std Z+8, r19
std Z+7, r19
...
...so by fairly random chance it happens to have decided to use r18/r19 instead of r20/r21, breaking the test, but I think that's arbitrary.

So I hand coded and changed the FileCheck lines to...
; CHECK: ldi [[REG1:r[0-9]+]], 0
; CHECK: ldi [[REG2:r[0-9]+]], 0
; CHECK: std Z+8, [[REG2]]
; CHECK: std Z+7, [[REG1]]
; CHECK: std Z+6, [[REG2]]
; CHECK: std Z+5, [[REG1]]
; CHECK: std Z+4, [[REG2]]
; CHECK: std Z+3, [[REG1]]
; CHECK: std Z+2, [[REG2]]
; CHECK: std Z+1, [[REG1]]
...which is logically the same but more resilient.

My concern is that if we use the autogenerated version, then patch and update it afterwards, you will see the diffs as you propose, but the differences are essentially non functional/trivial, and we will end up with a more fragile unit test, which can break again in the future for not very important reasons, creating false fails that need debugging.

Whereas my version that's hand corrected is more resilient than the original test, just as sensitive, and fixes the issue.

In the end, you're the maintainer so if you say "do it anyway" then I will, but I thought I'd flag the reason why I think a small tweak to a hand coded version is better here.

One thing I can definitely do is get rid of a lot of the other changes in the dynalloca.ll unit test and only leave the one change that's necessary to make the test pass if you'd like?

(I plan to do some work on dynamic allocation at some point in the future as I have private patches that I'd like to upstream one day but we can leave that separate.)

I'll put together another PR with an absolute bare minimum change, take a look and see if you can live with that.

Carl

Carl,

Now I understand your idea and think your concern is reasonable. Let us continue our talk in the other PR.

benshi001 reviewed Oct 13, 2025

View reviewed changes

benshi001 requested changes Oct 13, 2025

View reviewed changes

carlos4242 closed this Oct 16, 2025

carlos4242 force-pushed the main branch from f173d48 to 2121bda Compare October 16, 2025 22:46

carlos4242 added a commit to carlos4242/llvm-project that referenced this pull request Oct 16, 2025

Create a test for issue llvm#163082.

6e1e37f

carlos4242 mentioned this pull request Oct 17, 2025

[AVR] Fix for issue #163015, occasional corruption in stack passed params #164001

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AVR] Fix for issue #163015, occasional corruption in stack passed params #163082

[AVR] Fix for issue #163015, occasional corruption in stack passed params #163082

Uh oh!

carlos4242 commented Oct 12, 2025

Uh oh!

benshi001 Oct 13, 2025

Uh oh!

benshi001 Oct 13, 2025

Uh oh!

benshi001 Oct 13, 2025

Uh oh!

benshi001 left a comment

Uh oh!

benshi001 commented Oct 13, 2025

Uh oh!

carlos4242 commented Oct 14, 2025

Uh oh!

carlos4242 commented Oct 16, 2025

Uh oh!

benshi001 commented Oct 17, 2025 •

edited

Loading

Uh oh!

carlos4242 commented Oct 17, 2025

Uh oh!

benshi001 commented Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,34 @@
		; RUN: llc < %s -mtriple=avr \| FileCheck -v -dump-input always %s

[AVR] Fix for issue #163015, occasional corruption in stack passed params #163082

[AVR] Fix for issue #163015, occasional corruption in stack passed params #163082

Uh oh!

Conversation

carlos4242 commented Oct 12, 2025

Uh oh!

benshi001 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

benshi001 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

benshi001 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

benshi001 left a comment

Choose a reason for hiding this comment

Uh oh!

benshi001 commented Oct 13, 2025

Uh oh!

carlos4242 commented Oct 14, 2025

Uh oh!

carlos4242 commented Oct 16, 2025

Uh oh!

benshi001 commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlos4242 commented Oct 17, 2025

Uh oh!

benshi001 commented Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

benshi001 commented Oct 17, 2025 •

edited

Loading