[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:
> timshen wrote:
> > 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?
> I'm not sure I follow why the node would be transformed to something else if type legalization will legalize the type to `LegalizedType` and we've already determined that the operation is legal.
> 
> In any case, it seems like we might want to run this combine only after legalization. I think that in that case, we'd get the semantics we're after. And I think it makes sense for other targets too as the target may not be able to definitively say whether it supports modulo shift semantics until it knows everything about the operation and type.
Agreed. I move the legalization check to the caller sides.


https://reviews.llvm.org/D28329