[PATCH] D30571: [ARM] [Assembler] Extend implicit substitutions

[Javed Absar via Phabricator via llvm-commits]

javed.absar added a comment.

Hi Sanne:
Thanks for the patch. Please find some comments inline.
Best Regards
Javed



================
Comment at: lib/Target/ARM/ARM.td:264
 
+def FeatureNoImplicitSubstitutions : SubtargetFeature<"no-implicit-subs",
+                                        "ImplicitSubstitutions", "false",
----------------
If the substitution is only for instructions with immediates, would a narrower name e.g. FeatureNoImplicitImmSubstitution be more appropriate?


================
Comment at: lib/Target/ARM/ARMInstrThumb.td:1244
 
+def : tInstSubst<"add${s}${p} $rd, $rn, $imm",
+                 (tSUBi3 tGPR:$rd, s_cc_out:$s, tGPR:$rn, mod_imm1_7_neg:$imm, pred:$p)>;
----------------
should this be imm3 instead?


================
Comment at: test/MC/ARM/implicit-substitutions.s:116
+# CHECK-DISABLED: error: instruction requires: ImplicitSubstitutions
+# CHECK-DISABLED: MVN
+	SBC r0, r1, #0xFFFFFF00
----------------
You may consider shortening this test by reducing 'CHECK-DISABLED: error:  instruction requires ...' specially if there are duplication.  Related question - is the feature flag just for testing?


https://reviews.llvm.org/D30571