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

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>

On 07/07/2014 09:47 PM, deadal nix wrote:
> 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.
No, it is necessary and is the fundamental change in that commit. 
SelectVT is not necessarily the same as VT, so it needs to be converted. 
This patch breaks the testcases I have that this fixed. If it helps, 
this is the testcase I was fixing.

define i32 @test_sext_icmp_i64(i64 %arg) {
   %x = icmp eq i64 %arg, 0
   %y = sext i1 %x to i32
   ret i32 %y
}

The setcc type is i64 for the i64 icmp, so it then needs to be truncated 
to 32-bits in this situation

>
>
> 2014-07-07 11:36 GMT-07:00 Matt Arsenault <arsenm2 at gmail.com 
> <mailto:arsenm2 at gmail.com>>:
>
>
>     On Jul 5, 2014, at 7:14 PM, deadal nix <deadalnix at gmail.com
>     <mailto: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/20140708/855e4fba/attachment.html>

2014-07-08 12:11 GMT-07:00 Matt Arsenault <Matthew.Arsenault at amd.com>:
>  On 07/07/2014 09:47 PM, deadal nix wrote:
>
>  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.
>
> No, it is necessary and is the fundamental change in that commit. SelectVT
> is not necessarily the same as VT, so it needs to be converted. This patch
> breaks the testcases I have that this fixed. If it helps, this is the
> testcase I was fixing.
>
> define i32 @test_sext_icmp_i64(i64 %arg) {
>   %x = icmp eq i64 %arg, 0
>   %y = sext i1 %x to i32
>   ret i32 %y
> }
>
>
So this is not possible ?
(select (i64 setcc ...), (i32 -1), (i32 0))

>  The setcc type is i64 for the i64 icmp, so it then needs to be truncated
> to 32-bits in this situation
>
>
I understand this? Why does the setcc needs to be truncated ? It doesn't
look like it usually is.

I have a test case which is exactly the other way around :
define i32 @test_sext_icmp_i32(i32 %arg) {
  %x = icmp eq i32 %arg, 0
  %y = sext i1 %x to i64
  ret i64 %y
}

The DAG.getSExtOrTrunc(SetCC, DL, SelectVT) is returning the node
DAGCombiner is working on, which create a loop after the substitution. If I
have X = (i64 (sext (i32 (setcc A B)))) to be "combined" as (select X,
-1,
0) and then, after substitution occurs, the select become its own
condition, creating a loop in the DAG.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140708/36d94515/attachment.html>