llvm dev - Jul 2014 - [LLVMdev] LLVM commit 410f38e01597120b41e406ec1cea69127463f9e5

OK, so in you case, you want DAG.getSExtOrTrunc(SetCC, DL, SelectVT) to
tunc the result from i64 to i32 on 64 bits targets, if I understand
correctly.

2 questions:
 - Why not generating a selectcc node directly ? It avoid having to mess up
with intermediate values.
 - Why calling getSetCCResultType(VT) ? VT is not the type of a parameter
of setcc, and this looks incorrect to me.


2014-07-05 0:34 GMT-07:00 Matt Arsenault <arsenm2 at gmail.com>:
>
> On Jul 4, 2014, at 8:18 PM, deadal nix <deadalnix at gmail.com>
wrote:
>
> > Hi,
> >
> > I'm working on a target which have a variable size for CC (the
same size
> as the arguments). As a result getSetCCResultType, return a variable size.
> >
> > In this commit, at the line DAG.getSExtOrTrunc(SetCC, DL, SelectVT),
on
> my target, you end up generating the Node you are replacing, and so
> creating a loop in the DAG, which give a whole new meaning to the A in the
> acronym. Subsequent code manipulating the DAG to not like it at all.
> >
> > Can you explain me what you were trying to do in that commit ? I know
it
> is several month old, so the answer is likely not in cache, but that is
> capital to me to understand what is the correct fix.
> >
> > Thank,
> >
> > Amaury SECHET
>
> I was fixing creating a setcc with the wrong type for the operands for a
> target with the same problem, which would then hit a selection failure
> later.
>
> It was using getSetCCResultType on the result type of the SIGN_EXTEND
> node, rather than the types being compared in the setcc. The new setcc
> needs to have the right type, and then the result needs to be converted to
> the type of the sign_extend.
>
> I think in that case, it was something like (i64 sext (setcc (i32 x) (i32
> y)). getSetCCResultType is i64 for i64, and i32 for i32, so using the type
> of the sext created i64 (setcc (i32 x) (i32 y)) which doesn't work
>
> -Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140705/ea9c776e/attachment.html>

On Jul 5, 2014, at 7:14 PM, deadal nix <deadalnix at gmail.com> wrote:
> OK, so in you case, you want DAG.getSExtOrTrunc(SetCC, DL, SelectVT) to
tunc the result from i64 to i32 on 64 bits targets, if I understand correctly.
> 
> 2 questions:
>  - Why not generating a selectcc node directly ? It avoid having to mess up
with intermediate values.
Well first, that’s what it did originally and wasn’t what I was changing.
selectcc might not be legal, and I’m not sure why it even exists.  I don’t see
how it’s any harder to match select + setcc vs. selectcc, and selectcc just
gives you another case to worry about.

>  - Why calling getSetCCResultType(VT) ? VT is not the type of a parameter
of setcc, and this looks incorrect to me.
That’s what it did originally, and what I fixed. It now checks
getSetCCResultType of the operand’s operand’s value type, the setcc’s operand

OK from what I understand, the DAG.getSExtOrTrunc(SetCC, DL, SelectVT) is
unecessary and the SelectVT is nto the right type (as it is called with
incorrect parameter).

Here is a patch so it won't generate a loop. I ran make check and it
doesn't look like anything is broken.


2014-07-07 11:36 GMT-07:00 Matt Arsenault <arsenm2 at gmail.com>:
>
> On Jul 5, 2014, at 7:14 PM, deadal nix <deadalnix at gmail.com>
wrote:
>
> > OK, so in you case, you want DAG.getSExtOrTrunc(SetCC, DL, SelectVT)
to
> tunc the result from i64 to i32 on 64 bits targets, if I understand
> correctly.
> >
> > 2 questions:
> >  - Why not generating a selectcc node directly ? It avoid having to
mess
> up with intermediate values.
> Well first, that's what it did originally and wasn't what I was
changing.
> selectcc might not be legal, and I'm not sure why it even exists.  I
don't
> see how it's any harder to match select + setcc vs. selectcc, and
selectcc
> just gives you another case to worry about.
>
>
> >  - Why calling getSetCCResultType(VT) ? VT is not the type of a
> parameter of setcc, and this looks incorrect to me.
> That's what it did originally, and what I fixed. It now checks
> getSetCCResultType of the operand's operand's value type, the
setcc's
> operand
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140707/1d746cb3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Remove-loop-generation-in-DAGCombiner.patch
Type: text/x-patch
Size: 1069 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140707/1d746cb3/attachment.bin>