[ARM] Make i32 ISD::ABS Legal instead of pattern matching during isel.

[craig.topper]

Summary

Instead of matching (sra (X, size(X)-1); sub (xor (X, Y), Y))
during isel, make ISD::ABS Legal and use a regular isel pattern.

Also add new pseudo instructions for NABS to avoid creating a negate
after the ABS pseudo expansion. NABS is no longer expanded by DAGCombiner
to (sra (X, size(X)-1); sub (Y, xor (X, Y)) due to ABS being legal.
The new codegen looks like what gcc generates for NABS.

This is a followup from D119171.

[david-arm]

Hi @craig.topper, I'll be honest I haven't reviewed the code changes, but just looking at the tests it's not immediately obvious (to me at least!) what the benefit of this change is? Of course I could be missing something subtle here!

llvm/test/CodeGen/ARM/iabs.ll

50

I tested this out without your patch and I got this:

eor     r1, r0, r0, asr #31
rsb     r0, r1, r0, asr #31
bx      lr

which has the same number of instructions, but without any control flow.

llvm/test/CodeGen/Thumb2/abs.ll

44

It feels like this is actually now worse than before due to the presence of hardware control flow? Previously the eor.w and rsb instructions had no dependency on flags and also didn't set any.

[craig.topper]

In D120234#3335288, @david-arm wrote:

Hi @craig.topper, I'll be honest I haven't reviewed the code changes, but just looking at the tests it's not immediately obvious (to me at least!) what the benefit of this change is? Of course I could be missing something subtle here!

I don't understand it either. My goal was to remove the ABS matching code in ARMISelDAGToDAG.cpp which pre-dated the existence of the ISD::ABS node. After that I figured if the generated code for ABS using predicated RSB was better than the default expansion, then NABS should do the same thing using a different predicate.

[craig.topper]

Ping. Are there other reviews who can help understand if what is being done for ABS makes sense to extend to NABS?

[david-arm]

This patch hasn't moved for a long time. Trying to clean up my review list in Phabricator!

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp

 //===-- ARMISelDAGToDAG.cpp - A dag to dag inst selector for ARM ----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
void SelectVLDDup(SDNode *N, bool IsIntrinsic, bool isUpdating,
                     const uint16_t *QOpcodes0 = nullptr,
                     const uint16_t *QOpcodes1 = nullptr);
 
   /// Try to select SBFX/UBFX instructions for ARM.
   bool tryV6T2BitfieldExtractOp(SDNode *N, bool isSigned);
 
   bool tryInsertVectorElt(SDNode *N);
 
-  // Select special operations if node forms integer ABS pattern
-  bool tryABSOp(SDNode *N);
-
   bool tryReadRegister(SDNode *N);
   bool tryWriteRegister(SDNode *N);
 
   bool tryInlineAsm(SDNode *N);
 
   void SelectCMPZ(SDNode *N, bool &SwitchEQNEToPLMI);
 
   void SelectCMP_SWAP(SDNode *N);
SDValue Ops[] = { N->getOperand(0).getOperand(0),
                       getAL(CurDAG, dl), Reg0 };
     CurDAG->SelectNodeTo(N, Opc, MVT::i32, Ops);
     return true;
   }
 
   return false;
 }
 
-/// Target-specific DAG combining for ISD::SUB.
-/// Target-independent combining lowers SELECT_CC nodes of the form
-/// select_cc setg[ge] X,  0,  X, -X
-/// select_cc setgt    X, -1,  X, -X
-/// select_cc setl[te] X,  0, -X,  X
-/// select_cc setlt    X,  1, -X,  X
-/// which represent Integer ABS into:
-/// Y = sra (X, size(X)-1); sub (xor (X, Y), Y)
-/// ARM instruction selection detects the latter and matches it to
-/// ARM::ABS or ARM::t2ABS machine node.
-bool ARMDAGToDAGISel::tryABSOp(SDNode *N){
-  SDValue SUBSrc0 = N->getOperand(0);
-  SDValue SUBSrc1 = N->getOperand(1);
-  EVT VT = N->getValueType(0);
-
-  if (Subtarget->isThumb1Only())
-    return false;
-
-  if (SUBSrc0.getOpcode() != ISD::XOR || SUBSrc1.getOpcode() != ISD::SRA)
-    return false;
-
-  SDValue XORSrc0 = SUBSrc0.getOperand(0);
-  SDValue XORSrc1 = SUBSrc0.getOperand(1);
-  SDValue SRASrc0 = SUBSrc1.getOperand(0);
-  SDValue SRASrc1 = SUBSrc1.getOperand(1);
-  ConstantSDNode *SRAConstant =  dyn_cast<ConstantSDNode>(SRASrc1);
-  EVT XType = SRASrc0.getValueType();
-  unsigned Size = XType.getSizeInBits() - 1;
-
-  if (XORSrc1 == SUBSrc1 && XORSrc0 == SRASrc0 && XType.isInteger() &&
-      SRAConstant != nullptr && Size == SRAConstant->getZExtValue()) {
-    unsigned Opcode = Subtarget->isThumb2() ? ARM::t2ABS : ARM::ABS;
-    CurDAG->SelectNodeTo(N, Opcode, VT, XORSrc0);
-    return true;
-  }
-
-  return false;
-}
-
 /// We've got special pseudo-instructions for these
 void ARMDAGToDAGISel::SelectCMP_SWAP(SDNode *N) {
   unsigned Opcode;
   EVT MemTy = cast<MemSDNode>(N)->getMemoryVT();
   if (MemTy == MVT::i8)
     Opcode = Subtarget->isThumb() ? ARM::tCMP_SWAP_8 : ARM::CMP_SWAP_8;
   else if (MemTy == MVT::i16)
     Opcode = Subtarget->isThumb() ? ARM::tCMP_SWAP_16 : ARM::CMP_SWAP_16;
case ISD::READ_REGISTER:
     if (tryReadRegister(N))
       return;
     break;
   case ISD::INLINEASM:
   case ISD::INLINEASM_BR:
     if (tryInlineAsm(N))
       return;
     break;
-  case ISD::SUB:
-    // Select special operations if SUB node forms integer ABS pattern
-    if (tryABSOp(N))
-      return;
-    // Other cases are autogenerated.
-    break;
   case ISD::Constant: {
     unsigned Val = cast<ConstantSDNode>(N)->getZExtValue();
     // If we can't materialize the constant we need to use a literal pool
     if (ConstantMaterializationCost(Val, Subtarget) > 2) {
       SDValue CPIdx = CurDAG->getTargetConstantPool(
           ConstantInt::get(Type::getInt32Ty(*CurDAG->getContext()), Val),
           TLI->getPointerTy(CurDAG->getDataLayout()));
 

llvm/lib/Target/ARM/ARMISelLowering.cpp

 //===- ARMISelLowering.cpp - ARM DAG Lowering Implementation --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
for (unsigned im = (unsigned)ISD::PRE_INC;
       setIndexedStoreAction(im, MVT::i32, Legal);
     }
   } else {
     // Thumb-1 has limited post-inc load/store support - LDM r0!, {r1}.
     setIndexedLoadAction(ISD::POST_INC, MVT::i32,  Legal);
     setIndexedStoreAction(ISD::POST_INC, MVT::i32,  Legal);
   }
 
+  if (!Subtarget->isThumb1Only())
+    setOperationAction(ISD::ABS, MVT::i32, Legal);
+
   setOperationAction(ISD::SADDO, MVT::i32, Custom);
   setOperationAction(ISD::UADDO, MVT::i32, Custom);
   setOperationAction(ISD::SSUBO, MVT::i32, Custom);
   setOperationAction(ISD::USUBO, MVT::i32, Custom);
 
   setOperationAction(ISD::ADDCARRY, MVT::i32, Custom);
   setOperationAction(ISD::SUBCARRY, MVT::i32, Custom);
   if (Subtarget->hasDSP()) {
ARMTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
   case ARM::t2Int_eh_sjlj_setjmp_nofp:
     return BB;
 
   case ARM::Int_eh_sjlj_setup_dispatch:
     EmitSjLjDispatchBlock(MI, BB);
     return BB;
 
   case ARM::ABS:
-  case ARM::t2ABS: {
+  case ARM::NABS:
+  case ARM::t2ABS:
+  case ARM::t2NABS: {
     // To insert an ABS instruction, we have to insert the
     // diamond control-flow pattern.  The incoming instruction knows the
     // source vreg to test against 0, the destination vreg to set,
     // the condition code register to branch on, the
     // true/false values to select between, and a branch opcode to use.
     // It transforms
     //     V1 = ABS V0
     // into
case ARM::t2NABS: {
     RSBBB->addSuccessor(SinkBB);
 
     // insert a cmp at the end of BB
     BuildMI(BB, dl, TII->get(isThumb2 ? ARM::t2CMPri : ARM::CMPri))
         .addReg(ABSSrcReg)
         .addImm(0)
         .add(predOps(ARMCC::AL));
 
-    // insert a bcc with opposite CC to ARMCC::MI at the end of BB
+    // The condition code for ABS is MI, for NABS use GT.
+    bool IsNABS = MI.getOpcode() == ARM::NABS || MI.getOpcode() == ARM::t2NABS;
+    ARMCC::CondCodes CC = IsNABS ? ARMCC::GT : ARMCC::MI;
+
+    // insert a bcc with opposite CC at the end of BB
     BuildMI(BB, dl,
       TII->get(isThumb2 ? ARM::t2Bcc : ARM::Bcc)).addMBB(SinkBB)
-      .addImm(ARMCC::getOppositeCondition(ARMCC::MI)).addReg(ARM::CPSR);
+      .addImm(ARMCC::getOppositeCondition(CC)).addReg(ARM::CPSR);
 
     // insert rsbri in RSBBB
     // Note: BCC and rsbri will be converted into predicated rsbmi
     // by if-conversion pass
     BuildMI(*RSBBB, RSBBB->begin(), dl,
             TII->get(isThumb2 ? ARM::t2RSBri : ARM::RSBri), NewRsbDstReg)
         .addReg(ABSSrcReg, ABSSrcKIll ? RegState::Kill : 0)
         .addImm(0)

llvm/lib/Target/ARM/ARMInstrInfo.td

def SB : AInoP<(outs), (ins), MiscFrm, NoItinerary, "sb", "", []>,
          Requires<[IsARM, HasSB]>, Sched<[]> {
   let Inst{31-0} = 0xf57ff070;
   let Unpredictable = 0x000fff0f;
   let hasSideEffects = 1;
 }
 
 let usesCustomInserter = 1, Defs = [CPSR], hasNoSchedulingInfo = 1 in {
   // Pseudo instruction that combines movs + predicated rsbmi
-  // to implement integer ABS
-  def ABS : ARMPseudoInst<(outs GPR:$dst), (ins GPR:$src), 8, NoItinerary, []>;
+  // to implement integer ABS. Or movs + predicated rsbgt for NABS.
+  def ABS : ARMPseudoInst<(outs GPR:$dst), (ins GPR:$src), 8, NoItinerary,
+                          [(set GPR:$dst, (abs GPR:$src))]>;
+  def NABS : ARMPseudoInst<(outs GPR:$dst), (ins GPR:$src), 8, NoItinerary,
+                           [(set GPR:$dst, (ineg (abs GPR:$src)))]>;
 }
 
 let usesCustomInserter = 1, Defs = [CPSR], hasNoSchedulingInfo = 1 in {
     def COPY_STRUCT_BYVAL_I32 : PseudoInst<
       (outs), (ins GPR:$dst, GPR:$src, i32imm:$size, i32imm:$alignment),
       NoItinerary,
       [(ARMcopystructbyval GPR:$dst, GPR:$src, imm:$size, imm:$alignment)]>;
 }

llvm/lib/Target/ARM/ARMInstrThumb2.td

 let canFoldAsLoad = 1, isReMaterializable = 1 in
 def t2LDRpci_pic : PseudoInst<(outs rGPR:$dst), (ins i32imm:$addr, pclabel:$cp),
                    IIC_iLoadiALU,
               [(set rGPR:$dst, (ARMpic_add (load (ARMWrapper tconstpool:$addr)),
                                            imm:$cp))]>,
                Requires<[IsThumb2]>;
 
 // Pseudo instruction that combines movs + predicated rsbmi
-// to implement integer ABS
+// to implement integer ABS. Or movs + predicated rsbgt for NABS.
 let usesCustomInserter = 1, Defs = [CPSR], hasNoSchedulingInfo = 1 in {
 def t2ABS : PseudoInst<(outs rGPR:$dst), (ins rGPR:$src),
-                       NoItinerary, []>, Requires<[IsThumb2]>;
+                       NoItinerary, [(set rGPR:$dst, (abs rGPR:$src))]>,
+            Requires<[IsThumb2]>;
+def t2NABS : PseudoInst<(outs rGPR:$dst), (ins rGPR:$src),
+                        NoItinerary, [(set rGPR:$dst, (ineg (abs rGPR:$src)))]>,
+             Requires<[IsThumb2]>;
 }
 
 //===----------------------------------------------------------------------===//
 // Coprocessor load/store -- for disassembly only
 //
 class T2CI<bits<4> op31_28, dag oops, dag iops, string opc, string asm,
            list<dag> pattern, AddrMode am = AddrModeNone>
   : T2I<oops, iops, NoItinerary, opc, asm, pattern, am> {

llvm/test/CodeGen/ARM/iabs.ll

 ; CHECK-NEXT:    eor r0, r0, r1, asr #31
 ; CHECK-NEXT:    eor r1, r2, r1, asr #31
 ; CHECK-NEXT:    bx lr
   %tmp1neg = sub i64 0, %a
   %b = icmp sgt i64 %a, -1
   %abs = select i1 %b, i64 %a, i64 %tmp1neg
   ret i64 %abs
 }
+
+define i32 @test4(i32 %a) {
+; CHECK-LABEL: test4:
+; CHECK:       @ %bb.0:
+; CHECK-NEXT:    cmp r0, #0
+; CHECK-NEXT:    rsbgt r0, r0, #0
+; CHECK-NEXT:    bx lr
+        %tmp1neg = sub i32 0, %a
+        %b = icmp sgt i32 %a, -1
+        %abs = select i1 %b, i32 %tmp1neg, i32 %a
+        ret i32 %abs
+}

llvm/test/CodeGen/Thumb2/abs.ll

 ; CHECKT1:       @ %bb.0:
 ; CHECKT1-NEXT:    asrs r1, r0, #31
 ; CHECKT1-NEXT:    eors r0, r1
 ; CHECKT1-NEXT:    subs r0, r1, r0
 ; CHECKT1-NEXT:    bx lr
 ;
 ; CHECKT2-LABEL: neg_abs32:
 ; CHECKT2:       @ %bb.0:
-; CHECKT2-NEXT:    eor.w r1, r0, r0, asr #31
-; CHECKT2-NEXT:    rsb r0, r1, r0, asr #31
+; CHECKT2-NEXT:    cmp r0, #0
+; CHECKT2-NEXT:    it gt
+; CHECKT2-NEXT:    rsbgt r0, r0, #0
 ; CHECKT2-NEXT:    bx lr
   %abs = tail call i32 @llvm.abs.i32(i32 %x, i1 true)
   %neg = sub nsw i32 0, %abs
   ret i32 %neg
 }
 
 declare i16 @llvm.abs.i16(i16, i1 immarg)