[PATCH] D94054: [PowerPC] Try to fold sqrt/sdiv test results with the branch.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

It seems like this patch depends on another patch. Please don't forget to add the dependencies to the Phabricator reviews.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4384
 
+static bool isTestSqrtOpcode(SDValue N) {
+  if (N.getOpcode() == PPCISD::FTSQRT)
----------------
If the only possible use for these test nodes are to `and` the result with `1/2/4`, you might as well check the whole thing in this function.
Otherwise, I think it's fine as-is.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4390
+  switch (N.getConstantOperandVal(0)) {
+  case Intrinsic::ppc_vsx_xvtdivdp:
+  case Intrinsic::ppc_vsx_xvtdivsp:
----------------
Seems odd that a function called `isTestSqrtOpcode()` returns `true` for a test for SW divide.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4407
+  // (br_cc setne, (and TestOp, 8), 0) -> (BCC PRED_LT, TestOp)
+  SDValue CmpLHS = N->getOperand(2);
+  ISD::CondCode CC = cast<CondCodeSDNode>(N->getOperand(1))->get();
----------------
Please add an assert that `N` is the right type of node (i.e. `BR_CC`).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4409
+  ISD::CondCode CC = cast<CondCodeSDNode>(N->getOperand(1))->get();
+  if (!isa<ConstantSDNode>(N->getOperand(3)) ||
+      N->getConstantOperandVal(3) != 0 ||
----------------
It is quite hard to make sense of such huge condition and it aids readability to break it up and name the operands (i.e. `CC, CmpLHS, CmpRHS, TgtBlock`).

Then you can check the following in order and make it obvious:
- the RHS is zero
- the CC is EQ/NE
- the LHS is `(and (testSqrt), imm)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94054/new/

https://reviews.llvm.org/D94054