Remove possible loop creation in DAGCombiner

[deadal nix]

Which target ? I can try to figure out a solution. As per submitted test
cases, this do not affect X86 or X86-64 int that very scenario, the 32 bit
select is generated properly.

2014-09-25 13:57 GMT-07:00 Matt Arsenault <arsenm2 at gmail.com>:

>
> On Sep 25, 2014, at 1:42 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>
> > ----- Original Message -----
> >> From: "deadal nix" <deadalnix at gmail.com>
> >> To: "llvm-commits" <llvm-commits at cs.uiuc.edu>
> >> Sent: Thursday, September 25, 2014 12:09:07 AM
> >> Subject: Remove possible loop creation in DAGCombiner
> >>
> >>
> >>
> >>
> >> DAGCombiner can create loop in the DAG when you sext the result of a
> >> setcc, depending on the target.
> >>
> >> This diff remove the call to DAG.getSExtOrTrunc that create the loop.
> >> This call used to be necessary for x86 backend, but it doesn't look
> >> necessary anymore. I've added tests to ensure it doesn't.
> >
> > -        EVT SelectVT = getSetCCResultType(VT);
> > -        return DAG.getSelect(DL, VT,
> > -                             DAG.getSExtOrTrunc(SetCC, DL, SelectVT),
> > -                             NegOne, DAG.getConstant(0, VT));
> > -
> > +        return DAG.getSelect(DL, VT, SetCC,
> > +                          NegOne, DAG.getConstant(0, VT));
> >       }
> >
> > It seems clear to me that creating an SIGN_EXTEND inside of
> visitSIGN_EXTEND could easily lead to a loop, and we should avoid that.
> However, getSExtOrTrunc might also have been providing a TRUNC, and if the
> SIGN_EXTEND really would be needed, we may just want to abort the
> transformation all together. For one thing, I don't think that it can be
> different unless VT != N0.getOperand(0).getValueType().
>
> I do need the trunc behavior, for the case of selecting between 32-bit
> values based on a 64-bit comparison on a target which has the width of the
> setcc type change depending on the width of the compared operands
>
> >
> > In short, you're proposing to change target-independent code, and I'd be
> more conservative. Why don't you just disable the transformation when
> SelectVT.bitsGT(SetCC.getValueType()) is true (which I believe is the
> condition under which we'd get the SIGN_EXTEND).
>
> I agree, and what I was planning on trying when I got back to fixing that
> patch.
>
> >
> > -Hal
> >
> >>
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140926/53d5ed87/attachment.html>