Remove possible loop creation in DAGCombiner

[deadal nix]

I tried various options:
 - Not do the transformation when size do not match. This break several
backends as the sext is done from a 1bit value which most backend do not
expect or support. This needs to be transformed into a select one way or
another.
 - do the select with the setcc size, and then resize the select. It does
work but produce inefficient code in various situations (select a 64 bit
and resize the result to 32bits).
 - trunc but never extend. This yield the same results as the proposed diff.
 - the proposed diff.

Ultimately, the question is do the backend must handle slect with different
size for selected values and predicate. It look like they do (and other
part of the toolchain generate such code).

Another thing worth noticing is the use of getSetCCResultType with the
selected value's type as parameter. This is a different semantic than other
uses of the method (predicate's argument type is passed). It seems that
this is an incorrect use of that function to begin with, as there is no
reason that the 2 respond to the same constraint, and, in the case of the
select, this is not given that there is a constraint at all. Basing any
decision on the result of an incorrect use of that function will be a
source of trouble.

If such information is really required, another method should be used, but
as this is the only use case I've found in the codebase, and that other
part of it generate select without this, I'm inclined to assume that this
is not something commonly used. It is not used line 11422 in the same file
for instance.

2014-09-25 13:42 GMT-07:00 Hal Finkel <hfinkel at anl.gov>:

> ----- 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().
>
> 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).
>
>  -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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140926/0b6a5e4d/attachment.html>