-
Notifications
You must be signed in to change notification settings - Fork 981
createInlineAsm:avoid mapupdate collect after call #5009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
createInlineAsm:avoid mapupdate collect after call #5009
Conversation
fa7af34
to
73a1250
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably for @aykevl
registers[key] = b.getValue(r.Value.(*ssa.MakeInterface).X, getPos(instr)) | ||
case *ssa.Call: | ||
if r.Common() == instr { | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of extracting collectInlineAsmRegisters
, why not name the outer loop and use break <name>
? Something like:
referrers:
for _, r := range *registerMap.Referrers() {
...
if r.Common() == instr {
break referrers
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great suggestion! You're absolutely right - using labeled break is cleaner and more idiomatic. I've updated the code accordingly. I initially went with function extraction due to personal preference, but your approach is definitely better with fewer code changes. Thanks for the review!
6fd682d
to
07aa451
Compare
The test disappeared, was that on purpose? Also, please squash the two "goto" and "named break" commits into one. Probably @aykevl for the meat of this change. |
builder/createInlineAsm:with independant logic to avoid goto operate
07aa451
to
a996f85
Compare
I have merged the two commits into one. I temporarily removed the added tests to confirm whether the test failure issue is only coming from the CI environment. I will add back the tests shortly. |
702167d
to
2986a7a
Compare
Hey @aykevl! 👋 I found a little bug in TinyGo and managed to fix it - would love to get your eyes on the PR when you have a moment! |
fixed #5008
Problem:
The original code used a break statement inside a switch within a for loop, which only exited the switch but continued processing subsequent MapUpdate operations. This led to AsmFull returning register values from map updates that happened after the function call, violating the expected behavior.
Solution: