Page MenuHomePhabricator

[SVE][Inline-Asm] Add support to specify SVE registers in the clobber list

Authored by kmclaughlin on Jul 15 2019, 6:44 AM.

Diff Detail


Event Timeline

kmclaughlin created this revision.Jul 15 2019, 6:44 AM
Herald added a project: Restricted Project. · View Herald Transcript
kmclaughlin added a subscriber: cfe-commits.

Functionally the patch looks good, but the title suggests this adds full inline-asm support for SVE (which would require the ACLE types proposed in D62960, as well as other changes), where this patch only adds support to specify SVE registers in the clobber list.

4 ↗(On Diff #209845)

nit: there is no reason to have a different code-style for code and tests (curly brace is on next line here).
Maybe run this through clang-format?

7 ↗(On Diff #209845)

nit: The asm/instructions here don't really need to make sense (as in: they are not executed), so you can combine all three tests into one, as long as the instructions are valid and z0, p0, z31 and z15 are used.

rovka added a subscriber: rovka.Jul 18 2019, 2:50 AM
rovka added inline comments.
307 ↗(On Diff #209845)

You should commit the typo fixes separately.

1 ↗(On Diff #209845)

Can you also add a test without +sve, to make sure we get a diagnostic?

kmclaughlin retitled this revision from [SVE][Inline-Asm] Add support to clang for SVE inline assembly to [SVE][Inline-Asm] Add support to specify SVE registers in the clobber list.
  • Removed typo fixes from this patch
  • Consolidated the tests in aarch64-sve-inline-asm.c into one test & fixed formatting
  • Added a test without +sve to ensure we get the correct diagnostic
sdesmalen added inline comments.Jul 22 2019, 12:56 AM
12 ↗(On Diff #210821)

There is no variable linked to t in this inline asm, so you can remove this clause.

1 ↗(On Diff #209845)

Without the -emit-llvm part this test invokes (and tests the diagnostic of) the compiler. I don't think this is what we want. At the same time, this code should probably still continue match the z and p registers even if the target feature is not given, and thus leave it to LLVM to determine whether the use of these registers makes sense or not. So removing -target-feature +sve from the RUN line should be sufficient here. @rovka do you agree?

rovka added inline comments.Jul 22 2019, 3:44 AM
1 ↗(On Diff #209845)

Good point, we probably don't want this to be an integration test of the whole compiler. Removing the target feature altogether sounds good to me.

  • Updated test to remove unused variable 't'
  • Removed second test in aarch64-sve-inline-asm.c which didn't use -emit-llvm
This revision is now accepted and ready to land.Jul 23 2019, 2:20 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2019, 1:43 AM