[PATCH] D28329: [PowerPC, DAGCombiner] Fold a << (b % (sizeof(a) * 8)) back to a single instruction

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:1004
+             "Expect a shift instruction");
+      return isOperationLegal(Inst, ReturnType) && ReturnType.isVector();
+    }
----------------
nemanjai wrote:
> I think this will exclude the scalar versions as well as any that will be scalarized. However, it'll also exclude versions that will be legalized in different ways (i.e. without scalarization). Wouldn't we want something that more closely reflects what we are looking for (i.e. the type will remain a vector after legalization)? Perhaps something along the lines of:
> 
> ```
> Index: lib/Target/PowerPC/PPCISelLowering.h
> ===================================================================
> --- lib/Target/PowerPC/PPCISelLowering.h        (revision 290968)
> +++ lib/Target/PowerPC/PPCISelLowering.h        (working copy)
> @@ -996,6 +996,14 @@ namespace llvm {
>      SDValue
>        combineElementTruncationToVectorTruncation(SDNode *N,
>                                                   DAGCombinerInfo &DCI) const;
> +
> +    bool supportsModuloShift(ISD::NodeType Inst, EVT ReturnType,
> +                             const SelectionDAG &DAG) const override {
> +      assert((Inst == ISD::SHL || Inst == ISD::SRA || Inst == ISD::SRL) &&
> +             "Expect a shift instruction");
> +      EVT LegalizedType = getTypeToTransformTo(*DAG.getContext(), ReturnType);
> +      return isOperationLegal(Inst, LegalizedType) && LegalizedType.isVector();
> +    }
>    };
>  
>    namespace PPC {
> ```
> +      EVT LegalizedType = getTypeToTransformTo(*DAG.getContext(), ReturnType);
> +      return isOperationLegal(Inst, LegalizedType) && LegalizedType.isVector();

Let's say `isOperationLegal(Inst, LegalizedType) && LegalizedType.isVector();` is true, and we do the combine. However, later `Inst` turns into `Inst'` during the legalization, where `Inst'` might be any instruction that carries no modulo semantic. Won't that be a mis-combine?


https://reviews.llvm.org/D28329