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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

The description mentions i32/i64 explicitly, but there does not seem to be anything in the combine that narrows it down to those two types. In fact, it appears that it will combine it for any type (scalar or vector) as long as the MULH on the narrow type is legal.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15876
+         "Cannot have a multiply node with two different operand types.");
+
+  EVT NarrowVT = LeftOp.getOperand(0).getValueType();
----------------
You will probably need something like `(void)WideVT2;` to silence warnings on non-assert builds. Or you can just not define `WideVT2`, not have the assert and trust that the well-formedness of the multiply is adequately checked in target-independent code.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15888
+  SDValue ShiftAmtSrc = N->getOperand(1);
+  unsigned ShiftAmt =
+      isa<ConstantSDNode>(ShiftAmtSrc)
----------------
I don't understand this. Why do we assume that either `ShiftAmtSrc` is constant or its second operand is constant? What node do you expect it to be if it is not constant?
Also, won't this crash on `(srl (mul (zext i32:%a to i64), (zext i32:%b to i64)), %c)`?

i.e. something like:
```
unsigned test(unsigned a, unsigned b, unsigned c) {
  return (unsigned) (((uint64_t)a * b) >> c);
}
```


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

https://reviews.llvm.org/D78272