[PATCH] D78272: [PowerPC] DAG Combine to transform shifts into multiply-high

[Amy Kwan via Phabricator via llvm-commits]

amyk marked 3 inline comments as done.
amyk added a comment.

@craig.topper Do you think common-ing out the X86/PPC parts to combine to mulh into the target independent combiner is a good idea?



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15882
+  unsigned NarrowVTSize = NarrowVT.getScalarSizeInBits();
+  if (WideVT1.getScalarSizeInBits() != 2 * NarrowVTSize)
+    return SDValue();
----------------
craig.topper wrote:
> SIGN_EXTEND_INREG will never pass this check will it? The input and output type for that are the same. There's an extra operand carrying the type to extend from.
That's a good point. I thought I had tests involving SIGN_EXTEND_INREG that works with this, but I realize now that I actually don't and they're all sign extends for this patch. I've decided to move the check for SIGN_EXTEND_INREG.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15904
+  SDValue Result = DAG.getNode(MulhOpcode, DL, NarrowVT, LeftOp.getOperand(0),
+                               RightOp.getOperand(0));
+  return (IsSignExt ? DAG.getSExtOrTrunc(Result, DL, WideVT1)
----------------
craig.topper wrote:
> I don't see a check that RightOp.getOperand(0) and LeftOp.getOperand(0) are the the same type?
That's true, thank you for pointing that out. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15905
+                               RightOp.getOperand(0));
+  return (IsSignExt ? DAG.getSExtOrTrunc(Result, DL, WideVT1)
+                    : DAG.getZExtOrTrunc(Result, DL, WideVT1));
----------------
craig.topper wrote:
> I think what extend to use at the end needs to be base of the shift opcode not the extend opcode. If its an SRL, you need to put 0s in the upper bits even if the multiply is MULHS.
You're right, I'll fix that. 


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

https://reviews.llvm.org/D78272