[PATCH] D30571: [ARM] [Assembler] Extend immediate conversions for A32, T32 and T16

[Sanne Wouda via Phabricator via llvm-commits]

sanwou01 marked 2 inline comments as done.
sanwou01 added a comment.

Hi Javed,

Thanks for the review. I have updated the naming to use "immediate conversions" throughout. I think this is a better name, thanks for pointing it out.

See further comments inline.
Thanks,
Sanne



================
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)>;
----------------
javed.absar wrote:
> should this be imm3 instead?
I don't think so?  $imm is just a name here: the allowed range that is  is controlled by the mod_imm1_7_neg below.


================
Comment at: test/MC/ARM/implicit-substitutions.s:116
+# CHECK-DISABLED: error: instruction requires: ImplicitSubstitutions
+# CHECK-DISABLED: MVN
+	SBC r0, r1, #0xFFFFFF00
----------------
javed.absar wrote:
> 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?
The repetition of "error: instruction requires: ImmediateConversions" is necessary to test that -mattr=+no-imm-conversions properly disables all conversions.

No, the feature flag is not just for testing. The flag is helpful for users who want their code to assemble to exactly what they wrote. I have added this justification to the commit message.



https://reviews.llvm.org/D30571