-
Notifications
You must be signed in to change notification settings - Fork 106
Improve inline asm support #1206
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
Conversation
Thanks a lot! I will review later today. CI failure is due to some issues with the recent |
} | ||
|
||
impl<'tcx> InlineAssemblyGenerator<'_, 'tcx> { | ||
fn allocate_registers(&mut self) { |
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.
I think this function can fail to find a valid allocation even if one exists in the presence of overlapping registers. I don't think it is a high priority to fix this though.
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.
In some cases, e.g. when reg_abcd
is used for inputs and reg
is used for inouts, then this will fail. But for most cases this naive allocator should suffice.
} | ||
|
||
impl<'tcx> InlineAssemblyGenerator<'_, 'tcx> { | ||
fn allocate_registers(&mut self) { |
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.
This isn't skipping clobbers for registers unavailable given the target features of the current function, right? They need to be skipped I think.
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.
This is respecting sess.target_features
but not function-local #[target_feature]
ones.
d8bf21f
to
450640f
Compare
Hmm GitHub seems to be displaying the commits in the wrong order. EDIT: It seems to be a glitch in my clock or git that causes wrong timestamp to be used. |
Enabling assembly for compiler-builtins gives the following linker error:
Generated assembly:
|
Ah, I think this is just missing |
Trying to get blog os work with this branch. Almost there it seems. Only got two inline asm from different codegen units with a conflicting name:
I think it will need to use the codegen unit name instead of function name as base name. |
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.
Going to merge this now. The issues with blog os are unrelated to the changes in this PR.
Fix part of #1204