- Notifications
You must be signed in to change notification settings - Fork1k
fix: 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 ourterms of service andprivacy 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?
Conversation
fa7af34 to73a1250Compare
eliasnaur left a comment
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: | ||
| ifr.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 extractingcollectInlineAsmRegisters, why not name the outer loop and usebreak <name>? Something like:
referrers:for_,r:=range*registerMap.Referrers() {...ifr.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 to07aa451Compareeliasnaur commentedAug 24, 2025
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 toa996f85Compareluoliwoshang commentedAug 25, 2025
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 to2986a7aCompareluoliwoshang commentedAug 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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! |
aykevl commentedSep 9, 2025
While I appreciate you fixing this, I would actually prefer if we could remove AsmFull entirely. It's not very well defined, and C (GCC/Clang) has a far more flexible way to write inline assembly. It's annoying to use, yes, but doesn't require TinyGo to know all the details of an architecture. Here is an example of how it is done in the WS2812 driver: |
luoliwoshang commentedSep 10, 2025
Thank you for your feedback and for sharing the WS2812 example. |
Uh oh!
There was an error while loading.Please reload this page.
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: