Target-dependent translation of inline asm #383
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ahomescu
reviewed
Apr 25, 2022
fw-immunant
force-pushed
the
fw/target-dependent-asm
branch
from
fead6b0 to
0eccb6b
Compare
|
I've fixed a couple more bugs and added tests which will also test other architectures (just arm for now) as much as their runtime environment permits. |
fw-immunant
force-pushed
the
fw/target-dependent-asm
branch
4 times, most recently
from
c2b16ed to
52de819
Compare
fw-immunant
changed the title
rinon
approved these changes
Apr 28, 2022
| asm.contains('$') | ||
| // Guess based on sigils used in AT&T assembly. This would be more | ||
| // robust if it stripped comments first. | ||
| asm.contains('$') || asm.contains('%') || asm.contains('(') |
There was a problem hiding this comment.
I'm a bit concerned that '(' is too broad and will end up with false positives.
There was a problem hiding this comment.
'(' is intended to pick out disp(base,idx,scale) address calculations in AT&T syntax. Is there anywhere it can appear in Intel syntax? The only places I can think of are comments and macros. It's not hard to remove comments, but parsing macros seems like it's more trouble than we should bother with.
There was a problem hiding this comment.
We now remove comments before searching for these sigils.
fw-immunant
force-pushed
the
fw/target-dependent-asm
branch
from
f13b4b4 to
9771113
Compare
ahomescu
reviewed
Apr 29, 2022
not doing so could lead to a double-ampersand being emitted, which is not correct see #382
this is useful on the transpiler side for correctly interpreting inline asm
…isters for now we don't actually check specific reserved registers for non-x86 architectures, as I was not able to find any documentation listing which registers are reserved
…egister or index previously, if there were multiple inputs with similar but broad constraints, these would be incorrectly conflated and considered eligible for input-output association
this numbering follows the order used by gcc inline asm, i.e. outputs, inputs, then clobbers
…m machine constraints
fw-immunant
force-pushed
the
fw/target-dependent-asm
branch
3 times, most recently
from
b2d06ea to
505fe05
Compare
architecture-specific tests are denoted by ending their name with .$arch (such as .x86_64) and including a "target-tuple" file containing a target tuple on which the test is known to work. running tests on a host of the same architecture works as before; if the architectures differ, the target tuple is used to correctly transpile the test and (if an appropriate toolchain is present) compile the transpiled sources as well. lacking this toolchain is not considered a test failure.
remove comments before searching for sigils to determine if asm is AT&T or Intel syntax
support for these was added in Clang/LLVM 11, so this handling is #ifdef'd on that
for one major release, ElementCount's fields were public and not accessed via getters
like Half, this is basically just a stub for now supporting these types properly could use the `half` crate, but this is just to get basic AArch64 translation working this type was also added in Clang/LLVM 11, so ifdef'd accordingly we need to handle these now because ARMv8.6-a adds the BFloat16/__bf16 type as a vector element of one of the SVE vector types just added
fw-immunant
force-pushed
the
fw/target-dependent-asm
branch
from
505fe05 to
d4278f5
Compare
Closed
Co-authored-by: chrysn <[email protected]>
ahomescu
reviewed
May 2, 2022
| @@ -291,6 +498,12 @@ fn rewrite_asm<F: Fn(&str) -> bool>(asm: &str, is_mem_only: F) -> String { | |||
| continue; | |||
| } | |||
|
|
|||
| // Note empty chunks | |||
| if chunk == "" { | |||
There was a problem hiding this comment.
Minor nit: chunk.is_empty() could have worked too.
ahomescu
approved these changes
May 2, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This would benefit greatly from feedback on how it's detecting target architecture, which is currently pretty hacky and completely undocumented, and will also need some consideration on how we can test target-architecture-specific functionality in CI (I want to make sure installing toolchains for multiple architectures doesn't slow down CI as a whole too much, and we need to have a way to validate translation of programs for non-host architectures, perhaps without running them).