[ARM] Avoid using ARM instructions in Thumb mode

[samparker]

Summary

The Requires class overrides the target requirements of an instruction, rather than adding to them, so all ARM instructions need to include the IsARM predicate when they have overwitten requirements. This caused the swp and swpb instructions to be allowed in thumb mode assembly, and the ARM encoding of CDP to be selected in codegen (which is different for conditional instructions).

[rengolin]

Small comment about tests, but otherwise, looks good.

I'm slightly worried that SWP is deprecated in v6 and v7 and even optional in v7VE.

Not for this patch, but I'm wondering if we could make the instruction itself v4/v5 and have an InstAlias for v6/v7 so that we only generate is for old arches but still supports assembly with it.

Makes sense?
--renato

test/CodeGen/Thumb2/conditional-cdp.ll

1 ↗

(On Diff #86286)

It seems we're also missing ldc/stc/mcr etc. in the Thumb2 dir.

[samparker]

Hi Renato,

Yes, I will look into the v7VE issue; however, I'm not sure how using an InstAlias would be possible since the operation of SWP and LDREX/STREX pair seems different to my understanding. The STREX returns a store success flag while SWP can atomically swap the value between a register and a memory location. Am I misunderstanding the use of these instructions?

Cheers,
Sam

[rengolin]

In D29283#661489, @samparker wrote:

Yes, I will look into the v7VE issue;

Just to be clear, this can surely be for another patch. For this patch, just the additional tests on Thumb2 should do it.

however, I'm not sure how using an InstAlias would be possible since the operation of SWP and LDREX/STREX pair seems different to my understanding. The STREX returns a store success flag while SWP can atomically swap the value between a register and a memory location. Am I misunderstanding the use of these instructions?

They are different in syntax, yes, but being optional means we can't generate SWPs for v7VE. We already generate LDREX/STREX pairs, so I think it should be fine, and this change won't make it more likely to be generated in any way, so this is not impacting this patch.

This is just something to have in mind to do. Maybe a FIXME comment in the code would help.

--renato

[samparker]

Ok, will do. Just having a root around the tests, the missing Thumb2 tests appear to be in intrinsics-coprocessor.ll in the ARM folder instead.

cheers,
sam

[rengolin]

In D29283#661515, @samparker wrote:

Ok, will do. Just having a root around the tests, the missing Thumb2 tests appear to be in intrinsics-coprocessor.ll in the ARM folder instead.

Right. I'd either move the CDP test in there or duplicate all tests in the Thumb2 dir. Doesn't matter which way.

cheers,
--renato

[samparker]

Moved the Thumb2 tests into their own file, incorporating the new conditional cdp instruction. Added a fixme comment for SWP/SWPB.

[rengolin]

LGTM. Thanks!

llvm/trunk/include/llvm/Object/ELFObjectFile.h

public:
   virtual elf_symbol_iterator_range getDynamicSymbolIterators() const = 0;
 
   elf_symbol_iterator_range symbols() const;
 
   static inline bool classof(const Binary *v) { return v->isELF(); }
 
   SubtargetFeatures getFeatures() const override;
 
-  SubtargetFeatures getMIPSFeatures() const override;
+  SubtargetFeatures getMIPSFeatures() const;
 
-  SubtargetFeatures getARMFeatures() const override;
+  SubtargetFeatures getARMFeatures() const;
 
   void setARMSubArch(Triple &TheTriple) const override;
 };
 
 class ELFSectionRef : public SectionRef {
 public:
   ELFSectionRef(const SectionRef &B) : SectionRef(B) {
     assert(isa<ELFObjectFileBase>(SectionRef::getObject()));

llvm/trunk/include/llvm/Object/ObjectFile.h

public:
 
   /// @brief The number of bytes used to represent an address in this object
   ///        file format.
   virtual uint8_t getBytesInAddress() const = 0;
 
   virtual StringRef getFileFormatName() const = 0;
   virtual /* Triple::ArchType */ unsigned getArch() const = 0;
   virtual SubtargetFeatures getFeatures() const = 0;
-  virtual SubtargetFeatures getMIPSFeatures() const { return SubtargetFeatures(); }
-  virtual SubtargetFeatures getARMFeatures() const { return SubtargetFeatures(); }
   virtual void setARMSubArch(Triple &TheTriple) const { }
 
   /// Returns platform-specific object flags, if any.
   virtual std::error_code getPlatformFlags(unsigned &Result) const {
     Result = 0;
     return object_error::invalid_file_type;
   }
 

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

let AddedComplexity = 8 in {
   def : ARMPat<(atomic_load_acquire_8 addr_offset_none:$addr),  (LDAB addr_offset_none:$addr)>;
   def : ARMPat<(atomic_load_acquire_16 addr_offset_none:$addr), (LDAH addr_offset_none:$addr)>;
   def : ARMPat<(atomic_load_acquire_32 addr_offset_none:$addr), (LDA  addr_offset_none:$addr)>;
   def : ARMPat<(atomic_store_release_8 addr_offset_none:$addr, GPR:$val),  (STLB GPR:$val, addr_offset_none:$addr)>;
   def : ARMPat<(atomic_store_release_16 addr_offset_none:$addr, GPR:$val), (STLH GPR:$val, addr_offset_none:$addr)>;
   def : ARMPat<(atomic_store_release_32 addr_offset_none:$addr, GPR:$val), (STL  GPR:$val, addr_offset_none:$addr)>;
 }
 
-// SWP/SWPB are deprecated in V6/V7.
+// SWP/SWPB are deprecated in V6/V7 and optional in v7VE.
+// FIXME Use InstAlias to generate LDREX/STREX pairs instead.
 let mayLoad = 1, mayStore = 1 in {
 def SWP : AIswp<0, (outs GPRnopc:$Rt),
                 (ins GPRnopc:$Rt2, addr_offset_none:$addr), "swp", []>,
-                Requires<[PreV8]>;
+                Requires<[IsARM,PreV8]>;
 def SWPB: AIswp<1, (outs GPRnopc:$Rt),
                 (ins GPRnopc:$Rt2, addr_offset_none:$addr), "swpb", []>,
-                Requires<[PreV8]>;
+                Requires<[IsARM,PreV8]>;
 }
 
 //===----------------------------------------------------------------------===//
 // Coprocessor Instructions.
 //
 
 def CDP : ABI<0b1110, (outs), (ins p_imm:$cop, imm0_15:$opc1,
             c_imm:$CRd, c_imm:$CRn, c_imm:$CRm, imm0_7:$opc2),
             NoItinerary, "cdp", "\t$cop, $opc1, $CRd, $CRn, $CRm, $opc2",
             [(int_arm_cdp imm:$cop, imm:$opc1, imm:$CRd, imm:$CRn,
                           imm:$CRm, imm:$opc2)]>,
-            Requires<[PreV8]> {
+            Requires<[IsARM,PreV8]> {
   bits<4> opc1;
   bits<4> CRn;
   bits<4> CRd;
   bits<4> cop;
   bits<3> opc2;
   bits<4> CRm;
 
   let Inst{3-0}   = CRm;
   let Inst{4}     = 0;
   let Inst{7-5}   = opc2;
   let Inst{11-8}  = cop;
   let Inst{15-12} = CRd;
   let Inst{19-16} = CRn;
   let Inst{23-20} = opc1;
 }
 
 def CDP2 : ABXI<0b1110, (outs), (ins p_imm:$cop, imm0_15:$opc1,
                c_imm:$CRd, c_imm:$CRn, c_imm:$CRm, imm0_7:$opc2),
                NoItinerary, "cdp2\t$cop, $opc1, $CRd, $CRn, $CRm, $opc2",
                [(int_arm_cdp2 imm:$cop, imm:$opc1, imm:$CRd, imm:$CRn,
                               imm:$CRm, imm:$opc2)]>,
-               Requires<[PreV8]> {
+               Requires<[IsARM,PreV8]> {
   let Inst{31-28} = 0b1111;
   bits<4> opc1;
   bits<4> CRn;
   bits<4> CRd;
   bits<4> cop;
   bits<3> opc2;
   bits<4> CRm;
 
def _OPTION : ACInoP<(outs),
     let Inst{11-8} = cop;
     let Inst{7-0} = option;
     let DecoderMethod = "DecodeCopMemInstruction";
   }
 }
 
 defm LDC   : LdStCop <1, 0, "ldc", [(int_arm_ldc imm:$cop, imm:$CRd, addrmode5:$addr)]>;
 defm LDCL  : LdStCop <1, 1, "ldcl", [(int_arm_ldcl imm:$cop, imm:$CRd, addrmode5:$addr)]>;
-defm LDC2  : LdSt2Cop<1, 0, "ldc2", [(int_arm_ldc2 imm:$cop, imm:$CRd, addrmode5:$addr)]>, Requires<[PreV8]>;
-defm LDC2L : LdSt2Cop<1, 1, "ldc2l", [(int_arm_ldc2l imm:$cop, imm:$CRd, addrmode5:$addr)]>, Requires<[PreV8]>;
+defm LDC2  : LdSt2Cop<1, 0, "ldc2", [(int_arm_ldc2 imm:$cop, imm:$CRd, addrmode5:$addr)]>, Requires<[IsARM,PreV8]>;
+defm LDC2L : LdSt2Cop<1, 1, "ldc2l", [(int_arm_ldc2l imm:$cop, imm:$CRd, addrmode5:$addr)]>, Requires<[IsARM,PreV8]>;
 
 defm STC   : LdStCop <0, 0, "stc", [(int_arm_stc imm:$cop, imm:$CRd, addrmode5:$addr)]>;
 defm STCL  : LdStCop <0, 1, "stcl", [(int_arm_stcl imm:$cop, imm:$CRd, addrmode5:$addr)]>;
-defm STC2  : LdSt2Cop<0, 0, "stc2", [(int_arm_stc2 imm:$cop, imm:$CRd, addrmode5:$addr)]>, Requires<[PreV8]>;
-defm STC2L : LdSt2Cop<0, 1, "stc2l", [(int_arm_stc2l imm:$cop, imm:$CRd, addrmode5:$addr)]>, Requires<[PreV8]>;
+defm STC2  : LdSt2Cop<0, 0, "stc2", [(int_arm_stc2 imm:$cop, imm:$CRd, addrmode5:$addr)]>, Requires<[IsARM,PreV8]>;
+defm STC2L : LdSt2Cop<0, 1, "stc2l", [(int_arm_stc2l imm:$cop, imm:$CRd, addrmode5:$addr)]>, Requires<[IsARM,PreV8]>;
 
 //===----------------------------------------------------------------------===//
 // Move between coprocessor and ARM core register.
 //
 
 class MovRCopro<string opc, bit direction, dag oops, dag iops,
                 list<dag> pattern>
   : ABI<0b1110, oops, iops, NoItinerary, opc,

 }
 
 def MCR2 : MovRCopro2<"mcr2", 0 /* from ARM core register to coprocessor */,
                       (outs),
                       (ins p_imm:$cop, imm0_7:$opc1, GPR:$Rt, c_imm:$CRn,
                            c_imm:$CRm, imm0_7:$opc2),
                       [(int_arm_mcr2 imm:$cop, imm:$opc1, GPR:$Rt, imm:$CRn,
                                      imm:$CRm, imm:$opc2)]>,
-                      Requires<[PreV8]>;
+                      Requires<[IsARM,PreV8]>;
 def : ARMInstAlias<"mcr2 $cop, $opc1, $Rt, $CRn, $CRm",
                    (MCR2 p_imm:$cop, imm0_7:$opc1, GPR:$Rt, c_imm:$CRn,
                          c_imm:$CRm, 0)>;
 def MRC2 : MovRCopro2<"mrc2", 1 /* from coprocessor to ARM core register */,
                       (outs GPRwithAPSR:$Rt),
                       (ins p_imm:$cop, imm0_7:$opc1, c_imm:$CRn, c_imm:$CRm,
                            imm0_7:$opc2), []>,
-                      Requires<[PreV8]>;
+                      Requires<[IsARM,PreV8]>;
 def : ARMInstAlias<"mrc2 $cop, $opc1, $Rt, $CRn, $CRm",
                    (MRC2 GPRwithAPSR:$Rt, p_imm:$cop, imm0_7:$opc1, c_imm:$CRn,
                          c_imm:$CRm, 0)>;
 
 def : ARMV5TPat<(int_arm_mrc2 imm:$cop, imm:$opc1, imm:$CRn,
                               imm:$CRm, imm:$opc2),
                 (MRC2 imm:$cop, imm:$opc1, imm:$CRn, imm:$CRm, imm:$opc2)>;
 

 def MRRC : MovRRCopro<"mrrc", 1 /* from coprocessor to ARM core register */,
                       (outs GPRnopc:$Rt, GPRnopc:$Rt2),
                       (ins p_imm:$cop, imm0_15:$opc1, c_imm:$CRm), []>;
 
 class MovRRCopro2<string opc, bit direction, dag oops, dag iops,
                   list<dag> pattern = []>
   : ABXI<0b1100, oops, iops, NoItinerary,
          !strconcat(opc, "\t$cop, $opc1, $Rt, $Rt2, $CRm"), pattern>,
-    Requires<[PreV8]> {
+    Requires<[IsARM,PreV8]> {
   let Inst{31-28} = 0b1111;
   let Inst{23-21} = 0b010;
   let Inst{20} = direction;
 
   bits<4> Rt;
   bits<4> Rt2;
   bits<4> cop;
   bits<4> opc1;

llvm/trunk/test/CodeGen/ARM/2009-05-18-InlineAsmMem.ll

 ; RUN: llc -mtriple=arm-eabi %s -o - | FileCheck %s
-; RUN: llc -mtriple=thumb-eabi %s -o - | FileCheck %s
 ; PR4091
 
 define void @foo(i32 %i, i32* %p) nounwind {
 ;CHECK: swp r2, r0, [r1]
   %asmtmp = call i32 asm sideeffect "swp $0, $2, $3", "=&r,=*m,r,*m,~{memory}"(i32* %p, i32 %i, i32* %p) nounwind
   ret void
 }

llvm/trunk/test/CodeGen/ARM/intrinsics-coprocessor.ll

 ; RUN: llc < %s -mtriple=armv7-eabi -mcpu=cortex-a8 | FileCheck %s
-; RUN: llc < %s -march=thumb -mtriple=thumbv7-eabi -mcpu=cortex-a8 | FileCheck %s
 
 define void @coproc(i8* %i) nounwind {
 entry:
   ; CHECK: mrc p7, #1, r{{[0-9]+}}, c1, c1, #4
   %0 = tail call i32 @llvm.arm.mrc(i32 7, i32 1, i32 1, i32 1, i32 4) nounwind
   ; CHECK: mcr p7, #1, r{{[0-9]+}}, c1, c1, #4
   tail call void @llvm.arm.mcr(i32 7, i32 1, i32 %0, i32 1, i32 1, i32 4) nounwind
   ; CHECK: mrc2 p7, #1, r{{[0-9]+}}, c1, c1, #4

llvm/trunk/test/CodeGen/Thumb2/intrinsics-coprocessor.ll

+; RUN: llc < %s -march=thumb -mtriple=thumbv7-eabi -mcpu=cortex-a8 -show-mc-encoding | FileCheck %s
+define void @coproc(i8* %i) nounwind {
+entry:
+  ; CHECK: mrc p7, #1, r{{[0-9]+}}, c1, c1, #4
+  %0 = tail call i32 @llvm.arm.mrc(i32 7, i32 1, i32 1, i32 1, i32 4) nounwind
+  ; CHECK: mcr p7, #1, r{{[0-9]+}}, c1, c1, #4
+  tail call void @llvm.arm.mcr(i32 7, i32 1, i32 %0, i32 1, i32 1, i32 4) nounwind
+  ; CHECK: mrc2 p7, #1, r{{[0-9]+}}, c1, c1, #4
+  %1 = tail call i32 @llvm.arm.mrc2(i32 7, i32 1, i32 1, i32 1, i32 4) nounwind
+  ; CHECK: mcr2 p7, #1, r{{[0-9]+}}, c1, c1, #4
+  tail call void @llvm.arm.mcr2(i32 7, i32 1, i32 %1, i32 1, i32 1, i32 4) nounwind
+  ; CHECK: mcrr p7, #1, r{{[0-9]+}}, r{{[0-9]+}}, c1
+  tail call void @llvm.arm.mcrr(i32 7, i32 1, i32 %0, i32 %1, i32 1) nounwind
+  ; CHECK: mcrr2 p7, #1, r{{[0-9]+}}, r{{[0-9]+}}, c1
+  tail call void @llvm.arm.mcrr2(i32 7, i32 1, i32 %0, i32 %1, i32 1) nounwind
+  ; CHECK: cdp p7, #3, c1, c1, c1, #5
+  tail call void @llvm.arm.cdp(i32 7, i32 3, i32 1, i32 1, i32 1, i32 5) nounwind
+  ; CHECK: cdp2 p7, #3, c1, c1, c1, #5
+  tail call void @llvm.arm.cdp2(i32 7, i32 3, i32 1, i32 1, i32 1, i32 5) nounwind
+  ; CHECK: ldc p7, c3, [r{{[0-9]+}}]
+  tail call void @llvm.arm.ldc(i32 7, i32 3, i8* %i) nounwind
+  ; CHECK: ldcl p7, c3, [r{{[0-9]+}}]
+  tail call void @llvm.arm.ldcl(i32 7, i32 3, i8* %i) nounwind
+  ; CHECK: ldc2 p7, c3, [r{{[0-9]+}}]
+  tail call void @llvm.arm.ldc2(i32 7, i32 3, i8* %i) nounwind
+  ; CHECK: ldc2l p7, c3, [r{{[0-9]+}}]
+  tail call void @llvm.arm.ldc2l(i32 7, i32 3, i8* %i) nounwind
+  ; CHECK: stc p7, c3, [r{{[0-9]+}}]
+  tail call void @llvm.arm.stc(i32 7, i32 3, i8* %i) nounwind
+  ; CHECK: stcl p7, c3, [r{{[0-9]+}}]
+  tail call void @llvm.arm.stcl(i32 7, i32 3, i8* %i) nounwind
+  ; CHECK: stc2 p7, c3, [r{{[0-9]+}}]
+  tail call void @llvm.arm.stc2(i32 7, i32 3, i8* %i) nounwind
+  ; CHECK: stc2l p7, c3, [r{{[0-9]+}}]
+  tail call void @llvm.arm.stc2l(i32 7, i32 3, i8* %i) nounwind
+  ; CHECK: mrrc p1, #2, r{{[0-9]+}}, r{{[0-9]+}}, c3
+  %2 = tail call { i32, i32 } @llvm.arm.mrrc(i32 1, i32 2, i32 3) nounwind
+  ; CHECK: mrrc2 p1, #2, r{{[0-9]+}}, r{{[0-9]+}}, c3
+  %3 = tail call { i32, i32 } @llvm.arm.mrrc2(i32 1, i32 2, i32 3) nounwind
+  ret void
+}
+
+define hidden void @cond_cdp(i32 %a) {
+; CHECK-LABEL: cond_cdp:
+entry:
+  %tobool = icmp eq i32 %a, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:
+; CHECK: it ne
+; CHECK: cdpne   p15, #0, c0, c0, c0, #0 @ encoding: [0x00,0xee,0x00,0x0f]
+  tail call void @llvm.arm.cdp(i32 15, i32 0, i32 0, i32 0, i32 0, i32 0)
+  br label %if.end
+
+if.end:
+  ret void
+}
+
+declare void @llvm.arm.ldc(i32, i32, i8*) nounwind
+
+declare void @llvm.arm.ldcl(i32, i32, i8*) nounwind
+
+declare void @llvm.arm.ldc2(i32, i32, i8*) nounwind
+
+declare void @llvm.arm.ldc2l(i32, i32, i8*) nounwind
+
+declare void @llvm.arm.stc(i32, i32, i8*) nounwind
+
+declare void @llvm.arm.stcl(i32, i32, i8*) nounwind
+
+declare void @llvm.arm.stc2(i32, i32, i8*) nounwind
+
+declare void @llvm.arm.stc2l(i32, i32, i8*) nounwind
+
+declare void @llvm.arm.cdp2(i32, i32, i32, i32, i32, i32) nounwind
+
+declare void @llvm.arm.cdp(i32, i32, i32, i32, i32, i32) nounwind
+
+declare void @llvm.arm.mcrr2(i32, i32, i32, i32, i32) nounwind
+
+declare void @llvm.arm.mcrr(i32, i32, i32, i32, i32) nounwind
+
+declare void @llvm.arm.mcr2(i32, i32, i32, i32, i32, i32) nounwind
+
+declare i32 @llvm.arm.mrc2(i32, i32, i32, i32, i32) nounwind
+
+declare void @llvm.arm.mcr(i32, i32, i32, i32, i32, i32) nounwind
+
+declare i32 @llvm.arm.mrc(i32, i32, i32, i32, i32) nounwind
+
+declare { i32, i32 } @llvm.arm.mrrc(i32, i32, i32) nounwind
+
+declare { i32, i32 } @llvm.arm.mrrc2(i32, i32, i32) nounwind

llvm/trunk/test/MC/ARM/thumb2-diagnostics.s

@ CHECK-ERRORS: error: invalid operand for instruction
         strh r9, [pc, r10]
 @ CHECK-ERRORS: error: instruction requires: arm-mode
 @ CHECK-ERRORS: error: instruction requires: arm-mode
 @ CHECK-ERRORS: error: instruction requires: arm-mode
 @ CHECK-ERRORS: error: invalid operand for instruction
 @ CHECK-ERRORS: error: instruction requires: arm-mode
 @ CHECK-ERRORS: error: immediate value expected for vector index
 @ CHECK-ERRORS: error: instruction requires: arm-mode
+
+        @ SWP(B) is an ARM-only instruction
+        swp  r0, r1, [r2]
+        swpb r3, r4, [r5]
+@ CHECK-ERRORS: error: instruction requires: arm-mode
+@ CHECK-ERRORS: error: instruction requires: arm-mode