[PowerPC] Legalize SREM/UREM directly on P9.

[Esme]

Summary

As Bugzilla 35090 reported, the rationale for using custom lowering SREM/UREM should no longer be true. At the IR level, the div-rem-pairs pass rL312862 performs the transformation where the remainder is computed from the result of the division when both a required. We should now be able to lower these directly on P9. And the pass also fixed the problem that divide is in a different block than the remainder.
This is a NFC patch to remove redundant code.

[lkail]

It looks not an NFC patch to me, the test predicates are changed

CHECK: modsw -> CHECK-NOT: modsw

I'm curious if it is unchanged that opt is added to RUN lines while the OperationAction keeps Custom. If so, adding opt to RUN lines can be an NFC patch, and making OperationAction Legal can be another FC patch.

[Esme]

In D82145#2110509, @lkail wrote:

It looks not an NFC patch to me, the test predicates are changed

CHECK: modsw -> CHECK-NOT: modsw

I'm curious if it is unchanged that opt is added to RUN lines while the OperationAction keeps Custom. If so, adding opt to RUN lines can be an NFC patch, and making OperationAction Legal can be another FC patch.

Adding opt changed the predictions because the pass div-rem-pairs fixed the case @blocks_modulo_div_sw. If the pass is already added, making OperationAction Legal will not cause any change. I will separate the patch to 2 patches to make this clear.

[lkail]

Better keep original RUN lines and adding new RUN with opt added and check prefix named like CHECK-DIV-REM-PAIRS.

[Esme]

In D82145#2110583, @lkail wrote:

Better keep original RUN lines and adding new RUN with opt added and check prefix named like CHECK-DIV-REM-PAIRS.

I am not sure whether this is preferable. If so, the predictions will change after making OperationAction Legal...

[lkail]

I am not sure whether this is preferable. If so, the predictions will change after making OperationAction Legal...

Maybe I'm not making myself clear. We can make adding opt RUN lines in llvm/test/CodeGen/PowerPC/ppc64-P9-mod.ll an NFC patch

; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 -verify-machineinstrs | FileCheck %s
; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 -verify-machineinstrs | FileCheck %s
; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 | FileCheck %s -check-prefix=CHECK-PWR8 -implicit-check-not mod[us][wd]
; RUN: opt < %s -div-rem-pairs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 | \
; RUN:   llc -verify-machineinstrs | FileCheck -check-prefix=CHECK-DIV-REM-PAIRS %s
; RUN: opt < %s -div-rem-pairs -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 | \
; RUN:   llc -verify-machineinstrs | FileCheck -check-prefix=CHECK-DIV-REM-PAIRS %s
; RUN: opt < %s -div-rem-pairs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 | \
; RUN:   llc -verify-machineinstrs | FileCheck %s -check-prefix=CHECK-PWR8-DIV-REM-PAIRS -implicit-check-not mod[us][wd]

The second patch, we make actions Legal and compare the result of the NFC patch.

[Esme]

Update predictions for test cases.

[lkail]

LGTM.

[lkail]

Please remove 'NFC' from the title when landing this patch.

llvm/lib/Target/PowerPC/PPCISelLowering.h

     SDValue LowerSRL_PARTS(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerSRA_PARTS(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerBUILD_VECTOR(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerVECTOR_SHUFFLE(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerINSERT_VECTOR_ELT(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerEXTRACT_VECTOR_ELT(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerINTRINSIC_VOID(SDValue Op, SelectionDAG &DAG) const;
-    SDValue LowerREM(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerBSWAP(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerATOMIC_CMP_SWAP(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerSCALAR_TO_VECTOR(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerSIGN_EXTEND_INREG(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerMUL(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerABS(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerFP_EXTEND(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerROTL(SDValue Op, SelectionDAG &DAG) const;

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

   setOperationAction(ISD::FCEIL,  MVT::ppcf128, Expand);
   setOperationAction(ISD::FTRUNC, MVT::ppcf128, Expand);
   setOperationAction(ISD::FRINT,  MVT::ppcf128, Expand);
   setOperationAction(ISD::FNEARBYINT, MVT::ppcf128, Expand);
   setOperationAction(ISD::FREM, MVT::ppcf128, Expand);
 
   // PowerPC has no SREM/UREM instructions unless we are on P9
   // On P9 we may use a hardware instruction to compute the remainder.
-  // The instructions are not legalized directly because in the cases where the
-  // result of both the remainder and the division is required it is more
-  // efficient to compute the remainder from the result of the division rather
-  // than use the remainder instruction.
+  // When the result of both the remainder and the division is required it is
+  // more efficient to compute the remainder from the result of the division
+  // rather than use the remainder instruction. The instructions are legalized
+  // directly because the DivRemPairsPass performs the transformation at the IR
+  // level.
   if (Subtarget.isISA3_0()) {
-    setOperationAction(ISD::SREM, MVT::i32, Custom);
-    setOperationAction(ISD::UREM, MVT::i32, Custom);
-    setOperationAction(ISD::SREM, MVT::i64, Custom);
-    setOperationAction(ISD::UREM, MVT::i64, Custom);
+    setOperationAction(ISD::SREM, MVT::i32, Legal);
+    setOperationAction(ISD::UREM, MVT::i32, Legal);
+    setOperationAction(ISD::SREM, MVT::i64, Legal);
+    setOperationAction(ISD::UREM, MVT::i64, Legal);
   } else {
     setOperationAction(ISD::SREM, MVT::i32, Expand);
     setOperationAction(ISD::UREM, MVT::i32, Expand);
     setOperationAction(ISD::SREM, MVT::i64, Expand);
     setOperationAction(ISD::UREM, MVT::i64, Expand);
   }
 
   // Don't use SMUL_LOHI/UMUL_LOHI or SDIVREM/UDIVREM to lower SREM/UREM.

                    0);
   }
   default:
     break;
   }
   return SDValue();
 }
 
-SDValue PPCTargetLowering::LowerREM(SDValue Op, SelectionDAG &DAG) const {
-  // Check for a DIV with the same operands as this REM.
-  for (auto UI : Op.getOperand(1)->uses()) {
-    if ((Op.getOpcode() == ISD::SREM && UI->getOpcode() == ISD::SDIV) ||
-        (Op.getOpcode() == ISD::UREM && UI->getOpcode() == ISD::UDIV))
-      if (UI->getOperand(0) == Op.getOperand(0) &&
-          UI->getOperand(1) == Op.getOperand(1))
-        return SDValue();
-  }
-  return Op;
-}
-
 // Lower scalar BSWAP64 to xxbrd.
 SDValue PPCTargetLowering::LowerBSWAP(SDValue Op, SelectionDAG &DAG) const {
   SDLoc dl(Op);
   // MTVSRDD
   Op = DAG.getNode(ISD::BUILD_VECTOR, dl, MVT::v2i64, Op.getOperand(0),
                    Op.getOperand(0));
   // XXBRD
   Op = DAG.getNode(ISD::BSWAP, dl, MVT::v2i64, Op);

   case ISD::BITCAST:            return LowerBITCAST(Op, DAG);
 
   // Frame & Return address.
   case ISD::RETURNADDR:         return LowerRETURNADDR(Op, DAG);
   case ISD::FRAMEADDR:          return LowerFRAMEADDR(Op, DAG);
 
   case ISD::INTRINSIC_VOID:
     return LowerINTRINSIC_VOID(Op, DAG);
-  case ISD::SREM:
-  case ISD::UREM:
-    return LowerREM(Op, DAG);
   case ISD::BSWAP:
     return LowerBSWAP(Op, DAG);
   case ISD::ATOMIC_CMP_SWAP:
     return LowerATOMIC_CMP_SWAP(Op, DAG);
   }
 }
 
 void PPCTargetLowering::ReplaceNodeResults(SDNode *N,

llvm/test/CodeGen/PowerPC/ppc64-P9-mod.ll

-; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 -verify-machineinstrs | FileCheck %s
-; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 -verify-machineinstrs | FileCheck %s
-; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 | FileCheck %s -check-prefix=CHECK-PWR8 -implicit-check-not mod[us][wd]
+; RUN: opt < %s -div-rem-pairs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 | \
+; RUN:   llc -verify-machineinstrs | FileCheck %s
+; RUN: opt < %s -div-rem-pairs -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 | \
+; RUN:   llc -verify-machineinstrs | FileCheck %s
+; RUN: opt < %s -div-rem-pairs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 | \
+; RUN:   llc -verify-machineinstrs | FileCheck %s -check-prefix=CHECK-PWR8 -implicit-check-not mod[us][wd]
 
 @mod_resultsw = local_unnamed_addr global i32 0, align 4
 @mod_resultud = local_unnamed_addr global i64 0, align 8
 @div_resultsw = local_unnamed_addr global i32 0, align 4
 @mod_resultuw = local_unnamed_addr global i32 0, align 4
 @div_resultuw = local_unnamed_addr global i32 0, align 4
 @div_resultsd = local_unnamed_addr global i64 0, align 8
 @mod_resultsd = local_unnamed_addr global i64 0, align 8

 ; CHECK-PWR8-LABEL: const2_modulo_sw
 ; CHECK-PWR8: div
 ; CHECK-PWR8: mull
 ; CHECK-PWR8: sub
 ; CHECK-PWR8: blr
 }
 
 ; Function Attrs: norecurse nounwind
-; FIXME On power 9 this test will still produce modsw because the divide is in
-; a different block than the remainder. Due to the nature of the SDAG we cannot
-; see the div in the other block.
 define void @blocks_modulo_div_sw(i32 signext %a, i32 signext %b, i32 signext %c) local_unnamed_addr {
 entry:
   %div = sdiv i32 %a, %b
   store i32 %div, i32* @div_resultsw, align 4
   %cmp = icmp sgt i32 %c, 0
   br i1 %cmp, label %if.then, label %if.end
 
 if.then:                                          ; preds = %entry
   %rem = srem i32 %a, %b
   store i32 %rem, i32* @mod_resultsw, align 4
   br label %if.end
 
 if.end:                                           ; preds = %if.then, %entry
   ret void
 ; CHECK-LABEL: blocks_modulo_div_sw
+; CHECK-NOT: modsw
 ; CHECK: div
-; CHECK: modsw {{[0-9]+}}, 3, 4
+; CHECK-NOT: modsw
+; CHECK: mull
+; CHECK-NOT: modsw
+; CHECK: sub
 ; CHECK: blr
 ; CHECK-PWR8-LABEL: blocks_modulo_div_sw
 ; CHECK-PWR8: div
 ; CHECK-PWR8: mull
 ; CHECK-PWR8: sub
 ; CHECK-PWR8: blr
 }
 
-