[PATCH] D80485: [DAGCombiner][PowerPC][AArch64] Remove isMulhCheaperThanMulShift TLI hook. Use isOperationLegal directly instead.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D80485#2293942 <https://reviews.llvm.org/D80485#2293942>, @amyk wrote:

> So, my goal was to introduce a DAG combine to combine multiply+shifts into mulh. This is done in a function I introduced within `DAGCombiner.cpp` (called `combineShiftToMULH`).
>
> Since I was implementing something that is in target independent code, I thought it may be better to enable it on PowerPC only first since I was not quite sure if other targets were interested it this and I was seeing many other LIT failures at the time. 
> Thus, I introduced the hook (`isMulhCheaperThanMulShift`) and targets who wish to combine multiply+shifts into mulh could implement that hook. Within `combineShiftToMULH`, there is a check to `isMulhCheaperThanMulShift`.
>
> If we wanted to enable this for everyone, I think the check `isOperationLegalOrCustom` would probably be sufficient. I just tried this patch again, and made the following changes for `combineShiftToMULH`:
>
>   @@ -8099,12 +8101,6 @@ static SDValue combineShiftToMULH(SDNode *N, SelectionDAG &DAG,
>      if (NarrowVT !=  RightOp.getOperand(0).getValueType())
>        return SDValue();
>   
>   -  // Only transform into mulh if mulh for the narrow type is cheaper than
>   -  // a multiply followed by a shift. This should also check if mulh is
>   -  // legal for NarrowVT on the target.
>   -  if (!TLI.isMulhCheaperThanMulShift(NarrowVT))
>   -      return SDValue();
>   -
>      // Proceed with the transformation if the wide type is twice as large
>      // as the narrow type.
>      unsigned NarrowVTSize = NarrowVT.getScalarSizeInBits();
>   @@ -8122,6 +8118,12 @@ static SDValue combineShiftToMULH(SDNode *N, SelectionDAG &DAG,
>      // we use mulhs. Othewise, zero extends (zext) use mulhu.
>      unsigned MulhOpcode = IsSignExt ? ISD::MULHS : ISD::MULHU;
>   
>   +  // Only transform into mulh if mulh for the narrow type is cheaper than
>   +  // a multiply followed by a shift. This should also check if mulh is
>   +  // legal for NarrowVT on the target.
>   +  if (!TLI.isOperationLegalOrCustom(MulhOpcode, NarrowVT))
>   +      return SDValue();
>   +
>      SDValue Result = DAG.getNode(MulhOpcode, DL, NarrowVT, LeftOp.getOperand(0),
>                                   RightOp.getOperand(0));
>      return (N->getOpcode() == ISD::SRA ? DAG.getSExtOrTrunc(Result, DL, WideVT1)
>
> I only see one LIT failure now:
>
>   Failed Tests (1):
>     LLVM :: CodeGen/X86/pmulh.ll

Would this now be enabled on PPC32 or whatever !PPC64 is called? Are there tests for that?


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

https://reviews.llvm.org/D80485