[PATCH] D120259: [AArch64][AsmParser] Arch directives should set implied features.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/test/MC/AArch64/SVE/directive-arch_extension-negative.s:12
+
+ptrue   p0.b, pow2
+// CHECK: ptrue   p0.b, pow2
----------------
c-rhodes wrote:
> SVE is never enabled so I don't think this is doing what you intended, plus this instruction will be in the output for both pass/fail? I'm not sure it makes sense to mix positive/negative tests like this. As an aside, I'm not sure what my original intention was with this test but it would make sense to enable the feature before disabling it to verify nosve is doing what it's supposed to.
Good point!


================
Comment at: llvm/test/MC/AArch64/SVE/directive-arch_extension.s:7-12
+
+// Test that the implied +sve feature is also set from +sve2.
+.arch_extension sve2
+
+ptrue   p0.b, pow2
+// CHECK: ptrue   p0.b, pow2
----------------
c-rhodes wrote:
> unlike `.arch`, `.arch_extension` doesn't clear any previously selected architecture extensions, probably best to disable SVE with `.arch_extension nosve` first.
Good point!


================
Comment at: llvm/test/MC/AArch64/SVE/directive-cpu-negative.s:3
 
-.arch_extension nosve
+.cpu generic+nosve
 
----------------
c-rhodes wrote:
> enable SVE before disabling it?
Good suggestion, this actually exposed a bug in the existing implementation of .cpu directive!
https://godbolt.org/z/MndP97soh


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120259/new/

https://reviews.llvm.org/D120259