[PATCH] D33940: [Power9] Added support for the modsw, moduw, modsd, modud hardware instructions that are new to P9.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

Aside from a few minor inline nits, LGTM. Please address the inline comments on the commit.



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:207
 
-  // PowerPC has no SREM/UREM instructions
-  setOperationAction(ISD::SREM, MVT::i32, Expand);
-  setOperationAction(ISD::UREM, MVT::i32, Expand);
-  setOperationAction(ISD::SREM, MVT::i64, Expand);
-  setOperationAction(ISD::UREM, MVT::i64, Expand);
+  // PowerPC has no SREM/UREM instructions unless we are on P9
+  // On P9 we may use a harware instruction to compute the remainder.
----------------
jtony wrote:
> SREM/UREM are actually not instructions, they are SDNode or operations, I think.
Sure. Technically. But I think the comment is fine.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:8405
+SDValue PPCTargetLowering::LowerREM(SDValue Op, SelectionDAG &DAG) const {
+  // Check for a DIV that requires the same result as this REM.
+  SDNode* Denominator = Op.getOperand(1).getNode();
----------------
Well, this isn't completely accurate. Probably more accurate to say something like
`// Check for a DIV with the same operands as this REM.`


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:8406
+  // Check for a DIV that requires the same result as this REM.
+  SDNode* Denominator = Op.getOperand(1).getNode();
+  for (auto UI : Denominator->uses()) {
----------------
The temporary isn't really needed here. The loop can just be
`for (auto UI : Op.getOperand(1)->uses())`


================
Comment at: test/CodeGen/PowerPC/ppc64-P9-mod.ll:1
+; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 | FileCheck %s
+; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 | FileCheck %s -check-prefix=CHECK-PWR8 -implicit-check-not mod[us][wd]
----------------
jtony wrote:
> Do we need to test Big-endian also?
Yes, I agree. The codegen (and therefore CHECK directives) don't need to change, but please add a RUN line with triple `-mtriple=powerpc64-unknown-linux-gnu`.


================
Comment at: test/CodeGen/PowerPC/ppc64-P9-mod.ll:20
+; CHECK-LABEL: modulo_sw
+; CHECK: modsw
+; CHECK: blr
----------------
For the actual MOD instructions you're checking for where the operands are parameters, please add the input registers to the CHECK directives since they're mandated by the ABI. They should look something like
`; CHECK: modsw {{[0-9]+}}, 3, 4`

I don't think we care to do that for the checks of existing code though (i.e. the dif/mul/sub).


https://reviews.llvm.org/D33940