[ARM] Add 2-operand assembly aliases for Thumb1 ADD/SUB

[olista01]

Summary

This adds 2-operand assembly aliases for these instructions:

add r0, r1    =>   add r0, r0, r1
sub r0, r1    =>   sub r0, r0, r1

Previously this syntax was only accepted for Thumb2 targets, where the
wide versions of the instructions were used.

This patch allows the 2-operand syntax to be used for Thumb1 targets,
and selects the narrow encoding when it is used for Thumb2 targets.

[eastig]

LGTM.
BTW the issue was found during porting micropython for Cortex-M0 from GCC to Clang.

[rengolin]

As I said in D37374, please refrain from approving your own patches within minutes of posting. The community must be part of the process, and if people are not reviewing your patches as fast as you want, then there's either something wrong with the community (and we need to fix), or with your expectations.

If the problem is in the community, then the documented approach to make people care about your work is to care about theirs first. The more you review other people's patches, the more likely they are to review yours. If that doesn't work (after months), then we should discuss the problems in the dev list.

[eastig]

In D37377#858689, @rengolin wrote:

As I said in D37374, please refrain from approving your own patches within minutes of posting. The community must be part of the process, and if people are not reviewing your patches as fast as you want, then there's either something wrong with the community (and we need to fix), or with your expectations.

If the problem is in the community, then the documented approach to make people care about your work is to care about theirs first. The more you review other people's patches, the more likely they are to review yours. If that doesn't work (after months), then we should discuss the problems in the dev list.

Hi Renato,

I agree with you: other people from the community should also have a chance to review patches.

There might be my fault because I put my comment with the action 'Accept revision'. My understanding of the use of 'Accept revision' is that I have reviewed a patch and I accept it. Further steps, especially when to commit, should be done according to the LLVM developer policy. Maybe my understanding of this is wrong.

Sorry for the inconvenience caused.
-Evgeny

[rengolin]

In D37377#858713, @eastig wrote:

There might be my fault because I put my comment with the action 'Accept revision'. My understanding of the use of 'Accept revision' is that I have reviewed a patch and I accept it. Further steps, especially when to commit, should be done according to the LLVM developer policy. Maybe my understanding of this is wrong.

When you select "Accept" it means the patch can be committed without further reviews (unless you state clearly against it).

On same-company cases, due to conflict of interests as well as time-zone-wise, accepting a patch in less than 24-hours is really bad practice.

When in doubt, ask the code owner.

cheers,
--renato

[eastig]

Thank you, Renato, for clarifying that.

llvm/trunk/lib/Target/ARM/ARMInstrThumb.td

def tADDhirr : T1pIt<(outs GPR:$Rdn), (ins GPR:$Rn, GPR:$Rm), IIC_iALUr,
     bits<4> Rdn;
     bits<4> Rm;
     let Inst{7}   = Rdn{3};
     let Inst{6-3} = Rm;
     let Inst{2-0} = Rdn{2-0};
   }
 }
 
+def : tInstAlias <"add${s}${p} $Rdn, $Rm",
+                 (tADDrr tGPR:$Rdn,s_cc_out:$s, tGPR:$Rdn, tGPR:$Rm, pred:$p)>;
+
 def : tInstSubst<"sub${s}${p} $rd, $rn, $imm",
                  (tADDi3 tGPR:$rd, s_cc_out:$s, tGPR:$rn, mod_imm1_7_neg:$imm, pred:$p)>;
 def : tInstSubst<"sub${s}${p} $rdn, $imm",
                  (tADDi8 tGPR:$rdn, s_cc_out:$s, mod_imm8_255_neg:$imm, pred:$p)>;
 
 
 // AND register
 let isCommutable = 1 in

 // Subtract register
 def tSUBrr :                    // A8.6.212
   T1sIGenEncode<0b01101, (outs tGPR:$Rd), (ins tGPR:$Rn, tGPR:$Rm),
                 IIC_iALUr,
                 "sub", "\t$Rd, $Rn, $Rm",
                 [(set tGPR:$Rd, (sub tGPR:$Rn, tGPR:$Rm))]>,
                 Sched<[WriteALU]>;
 
+def : tInstAlias <"sub${s}${p} $Rdn, $Rm",
+                 (tSUBrr tGPR:$Rdn,s_cc_out:$s, tGPR:$Rdn, tGPR:$Rm, pred:$p)>;
+
 /// Similar to the above except these set the 's' bit so the
 /// instruction modifies the CPSR register.
 ///
 /// These opcodes will be converted to the real non-S opcodes by
 /// AdjustInstrPostInstrSelection after giving then an optional CPSR operand.
 let hasPostISelHook = 1, Defs = [CPSR] in {
   let Uses = [CPSR] in
   def tSBCS : tPseudoInst<(outs tGPR:$Rdn), (ins tGPR:$Rn, tGPR:$Rm),

llvm/trunk/test/MC/ARM/implicit-it-generation.s

@ The .rept directive can be used to create long IT blocks.
   .section test19
 @ CHECK-LABEL: test19
   .rept 3
   addeq r0, r1
   subne r0, r1
   .endr
 @ CHECK: itete eq
 @ CHECK:  addeq r0, r1
-@ CHECK:  subne.w r0, r0, r1
+@ CHECK:  subne r0, r0, r1
 @ CHECK:  addeq r0, r1
-@ CHECK:  subne.w r0, r0, r1
+@ CHECK:  subne r0, r0, r1
 @ CHECK: ite eq
 @ CHECK:  addeq r0, r1
-@ CHECK:  subne.w r0, r0, r1
+@ CHECK:  subne r0, r0, r1
 
 @ Flush at end of file
   .section test99
 @ CHECK-LABEL: test99
   addeq r0, #1
 @ CHECK: it eq
 @ CHECK: addeq

llvm/trunk/test/MC/ARM/thumb-add-sub-width.s

+// RUN: llvm-mc -triple thumbv6m -show-encoding < %s | FileCheck %s
+
+  .text
+  .thumb
+
+  // Check that the correct encoding of the add and sub instructions is
+  // selected, for all combinations of flag-setting, condition and 2- or
+  // 3-operand syntax.
+
+  .arch armv6-m
+  add r0, r0, r1     // T2
+  add r0, r1         // T2
+  adds r0, r0, r1    // T1
+  adds r0, r1        // T1
+// CHECK: add     r0, r1                  @ encoding: [0x08,0x44]
+// CHECK: add     r0, r1                  @ encoding: [0x08,0x44]
+// CHECK: adds    r0, r0, r1              @ encoding: [0x40,0x18]
+// CHECK: adds    r0, r0, r1              @ encoding: [0x40,0x18]
+
+  .arch armv7-m
+  add r0, r0, r1     // T2, T3
+  add r0, r1         // T2, T3
+  adds r0, r0, r1    // T1, T3
+  adds r0, r1        // T1, T3
+// CHECK: add     r0, r1                  @ encoding: [0x08,0x44]
+// CHECK: add     r0, r1                  @ encoding: [0x08,0x44]
+// CHECK: adds    r0, r0, r1              @ encoding: [0x40,0x18]
+// CHECK: adds    r0, r0, r1              @ encoding: [0x40,0x18]
+
+  itttt eq
+// CHECK: itttt   eq                      @ encoding: [0x01,0xbf]
+  addeq r0, r0, r1   // T1, T2, T3
+  addeq r0, r1       // T2, T1, T3
+  addseq r0, r0, r1  // T3
+  addseq r0, r1      // T3
+  // NOTE: Both T1 and T2 are valid for these two instructions, which one is
+  // the preferred varies depending on whether the 2- or 3-operand syntax was
+  // used.
+// CHECK: addeq   r0, r0, r1              @ encoding: [0x40,0x18]
+// CHECK: addeq   r0, r1                  @ encoding: [0x08,0x44]
+// CHECK: addseq.w        r0, r0, r1      @ encoding: [0x10,0xeb,0x01,0x00]
+// CHECK: addseq.w        r0, r0, r1      @ encoding: [0x10,0xeb,0x01,0x00]
+
+  .arch armv6-m
+  // NOTE: There is no non-flag-setting sub instruction for v6-M
+  subs r0, r0, r1    // T1, T2
+  subs r0, r1        // T1, T2
+// CHECK: subs    r0, r0, r1              @ encoding: [0x40,0x1a]
+// CHECK: subs    r0, r0, r1              @ encoding: [0x40,0x1a]
+
+  .arch armv7-m
+  sub r0, r0, r1     // T2
+  sub r0, r1         // T2
+  subs r0, r0, r1    // T1, T2
+  subs r0, r1        // T1, T2
+// CHECK: sub.w   r0, r0, r1              @ encoding: [0xa0,0xeb,0x01,0x00]
+// CHECK: sub.w   r0, r0, r1              @ encoding: [0xa0,0xeb,0x01,0x00]
+// CHECK: subs    r0, r0, r1              @ encoding: [0x40,0x1a]
+// CHECK: subs    r0, r0, r1              @ encoding: [0x40,0x1a]
+
+  itttt eq
+// CHECK: itttt   eq                      @ encoding: [0x01,0xbf]
+  subeq r0, r0, r1   // T1, T2
+  subeq r0, r1       // T1, T2
+  subseq r0, r0, r1  // T2
+  subseq r0, r1      // T2
+// CHECK: subeq   r0, r0, r1              @ encoding: [0x40,0x1a]
+// CHECK: subeq   r0, r0, r1              @ encoding: [0x40,0x1a]
+// CHECK: subseq.w        r0, r0, r1      @ encoding: [0xb0,0xeb,0x01,0x00]
+// CHECK: subseq.w        r0, r0, r1      @ encoding: [0xb0,0xeb,0x01,0x00]