llvm dev - Jan 2019 - Potential bug in SelectionDAGLegalize::ConvertNodeToLibcall()?

+ Eli Friedman as he often has very insightful comments regarding back end
changes.

On Fri, Jan 4, 2019 at 9:03 AM Nemanja Ivanovic <nemanja.i.ibm at
gmail.com>
wrote:
> The changes seem fine to me. I don't think this is excessively
intrusive
> and it accomplishes what is needed by targets whose call lowering can
> introduce illegal types.
> Adding Justin Bogner as the owner of SDAG and Hal Finkel as the PPC back
> end owner for their opinions.
>
> Nemanja
>
> On Thu, Jan 3, 2019 at 4:54 PM Justin Hibbits <jrh29 at
alumni.cwru.edu>
> wrote:
>
>> Hi Nemanja,
>>
>> I'm attaching a patch that builds on D54583 and implements what we
>> discussed on IRC earlier today.  Particularly:
>>
>> * Make LowerCallTo() a virtual function, so it can be wrapped by a
>>   subclass.
>> * Implement LowerCallTo() in PPCTargetLowering to wrap
>>   TargetLowering::LowerCallTo() and legalize the return node when
>>   targeting SPE.
>> * Augment PPCTargetLowering::LowerCall_32SVR4() to legalize MVT::f64
>>   arguments that have been pre-processed into
>>   EXTRACT_ELEMENT(i64 BITCAST f64, 0/1)
>>
>> The purpose of this being to legalize intermediate illegal types
>> post-type legalization.
>>
>> Is there a better approach?  Comments from anyone else?
>>
>> - Justin
>>
>> On Wed, 2 Jan 2019 11:39:59 -0500
>> Nemanja Ivanovic <nemanja.i.ibm at gmail.com> wrote:
>>
>> > It sounds like the legalizer is lowering `fmaxnum` to a libcall
>> > because it is not a legal node for `f64` and in doing so, it is
>> > producing the `build_pair` to reassemble the results of the
libcall.
>> > And presumably, it is assuming that the new nodes do not need
>> > legalization or something along those lines.
>> >
>> > Justin, it would probably be good if you could provide the DAG
before
>> > and after legalization both with and without your patch. Then we
can
>> > see how it was handled before your patch and how it is handled now
>> > and the difference should allude to the problem.
>> >
>> > N
>> >
>> > On Wed, Jan 2, 2019 at 10:54 AM Justin Hibbits via llvm-dev <
>> > llvm-dev at lists.llvm.org> wrote:
>> >
>> > > Hi,
>> > >
>> > > I have a custom lowering operation on ISD::BITCAST for the
>> > > PowerPC/SPE target, to convert 'f64 bitcast (i64
build_pair i32,
>> > > i32)' into a 'f64 BUILD_SPE64 i32, i32' node,
which can be seen at
>> > > https://reviews.llvm.org/D54583. However, when building
>> > > compiler-rt's lib/builtins/divdc3.c an assertion is
triggered that
>> > > BUILD_PAIR is not legal on line 24.  There should be no
>> > > bitcast(buildpair) anywhere in the generation, as it should
all be
>> > > lowered.  However, this is not the case when lowering to a
libcall
>> > > it seems.  The relevant debug output, is here:
>> > >
>> > > Creating new node: t118: i64 = build_pair t117,t116,
>> > >
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>> > > Creating new node: t119: f64 = bitcast t118,
>> > >
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>> > > Created libcall: t119: f64 = bitcast t118,
>> > >
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>> > > Successfully converted node to libcall
>> > >  ... replacing: t38: f64 = fmaxnum t36, t37,
>> > >
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>> > >      with:      t119: f64 = bitcast t118,
>> > >
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>> > >
>> > > Is this a real bug, or am I missing something in my patch? 
After
>> > > spending quite a while on it I'm at a loss.
>> > >
>> > > Thanks,
>> > > Justin
>> > > _______________________________________________
>> > > LLVM Developers mailing list
>> > > llvm-dev at lists.llvm.org
>> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> > >
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190104/54f2fe31/attachment-0001.html>

In general, we should not introduce nodes illegal types after type 
legalization; type legalization over a general DAG is complicated, so 
the type legalization code uses some specialized data structures.  
(Also, it's easier to reason about what operations can show up at 
various points in legalization.)

 From your description, with your patch, the bitcast is still created, 
but it is cleaned up before any other code sees it. That sort of works, 
but it's a hack.  We should avoid creating such nodes at all.

On ARM, we have special handling for f64 arguments on targets where f64 
is legal, but the calling convention is soft-float.  We use the existing 
argument lowering hooks to avoid generating nodes with illegal types, 
and instead generate target-specific nodes: ARMISD::VMOVRRD and 
ARMISD::VMOVDRR.  ARMISD::VMOVRRD takes two i32 values and produces an 
f64, and ARMISD::VMOVDRR takes an f64 and produces two i32 values.  This 
is implemented in ARMTargetLowering::LowerReturn, 
ARMTargetLowering::LowerFormalArguments, and 
ARMTargetLowering::LowerCall.  (See ARMTargetLowering::PassF64ArgInRegs 
etc.)

As far as I can tell, the SPE target is similar, so a similar approach 
should fix the issue, without any changes to target-independent code.

It would be possible to add some target-independent code to make this 
sort of handling a little easier.  We could define target-independent 
nodes which are equivalent to the ARMISD nodes I mentioned.  And if we 
had those nodes, we could add a target-independent helper for argument 
lowering that would generate them.  Not sure how much of that is 
actually worth doing; probably a lot of work to remove a small amount of 
redundant code.

-Eli

On 1/4/2019 6:05 AM, Nemanja Ivanovic wrote:
> + Eli Friedman as he often has very insightful comments regarding back 
> end changes.
>
> On Fri, Jan 4, 2019 at 9:03 AM Nemanja Ivanovic 
> <nemanja.i.ibm at gmail.com <mailto:nemanja.i.ibm at
gmail.com>> wrote:
>
>     The changes seem fine to me. I don't think this is excessively
>     intrusive and it accomplishes what is needed by targets whose call
>     lowering can introduce illegal types.
>     Adding Justin Bogner as the owner of SDAG and Hal Finkel as the
>     PPC back end owner for their opinions.
>
>     Nemanja
>
>     On Thu, Jan 3, 2019 at 4:54 PM Justin Hibbits
>     <jrh29 at alumni.cwru.edu <mailto:jrh29 at
alumni.cwru.edu>> wrote:
>
>         Hi Nemanja,
>
>         I'm attaching a patch that builds on D54583 and implements what
we
>         discussed on IRC earlier today.  Particularly:
>
>         * Make LowerCallTo() a virtual function, so it can be wrapped by a
>           subclass.
>         * Implement LowerCallTo() in PPCTargetLowering to wrap
>           TargetLowering::LowerCallTo() and legalize the return node when
>           targeting SPE.
>         * Augment PPCTargetLowering::LowerCall_32SVR4() to legalize
>         MVT::f64
>           arguments that have been pre-processed into
>           EXTRACT_ELEMENT(i64 BITCAST f64, 0/1)
>
>         The purpose of this being to legalize intermediate illegal types
>         post-type legalization.
>
>         Is there a better approach?  Comments from anyone else?
>
>         - Justin
>
>         On Wed, 2 Jan 2019 11:39:59 -0500
>         Nemanja Ivanovic <nemanja.i.ibm at gmail.com
>         <mailto:nemanja.i.ibm at gmail.com>> wrote:
>
>         > It sounds like the legalizer is lowering `fmaxnum` to a
libcall
>         > because it is not a legal node for `f64` and in doing so, it
is
>         > producing the `build_pair` to reassemble the results of the
>         libcall.
>         > And presumably, it is assuming that the new nodes do not need
>         > legalization or something along those lines.
>         >
>         > Justin, it would probably be good if you could provide the
>         DAG before
>         > and after legalization both with and without your patch.
>         Then we can
>         > see how it was handled before your patch and how it is
>         handled now
>         > and the difference should allude to the problem.
>         >
>         > N
>         >
>         > On Wed, Jan 2, 2019 at 10:54 AM Justin Hibbits via llvm-dev
<
>         > llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>         >
>         > > Hi,
>         > >
>         > > I have a custom lowering operation on ISD::BITCAST for
the
>         > > PowerPC/SPE target, to convert 'f64 bitcast (i64
>         build_pair i32,
>         > > i32)' into a 'f64 BUILD_SPE64 i32, i32' node,
which can be
>         seen at
>         > > https://reviews.llvm.org/D54583. However, when building
>         > > compiler-rt's lib/builtins/divdc3.c an assertion is
>         triggered that
>         > > BUILD_PAIR is not legal on line 24.  There should be no
>         > > bitcast(buildpair) anywhere in the generation, as it
>         should all be
>         > > lowered.  However, this is not the case when lowering to
a
>         libcall
>         > > it seems.  The relevant debug output, is here:
>         > >
>         > > Creating new node: t118: i64 = build_pair t117,t116,
>         > >
>        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>         > > Creating new node: t119: f64 = bitcast t118,
>         > >
>        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>         > > Created libcall: t119: f64 = bitcast t118,
>         > >
>        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>         > > Successfully converted node to libcall
>         > >  ... replacing: t38: f64 = fmaxnum t36, t37,
>         > >
>        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>         > >      with:      t119: f64 = bitcast t118,
>         > >
>        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
>         > >
>         > > Is this a real bug, or am I missing something in my
>         patch?  After
>         > > spending quite a while on it I'm at a loss.
>         > >
>         > > Thanks,
>         > > Justin
>         > > _______________________________________________
>         > > LLVM Developers mailing list
>         > > llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>         > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>         > >
>
-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190107/e1c876d3/attachment.html>

Hi Eli,

Thanks for this, especially the ARM pointer.  I'll take a look and try
to have something better in the next week or so.

- Justin

On Mon, 7 Jan 2019 12:57:14 -0800
"Friedman, Eli" <efriedma at codeaurora.org> wrote:
> In general, we should not introduce nodes illegal types after type 
> legalization; type legalization over a general DAG is complicated, so 
> the type legalization code uses some specialized data structures.  
> (Also, it's easier to reason about what operations can show up at 
> various points in legalization.)
> 
>  From your description, with your patch, the bitcast is still
> created, but it is cleaned up before any other code sees it. That
> sort of works, but it's a hack.  We should avoid creating such nodes
> at all.
> 
> On ARM, we have special handling for f64 arguments on targets where
> f64 is legal, but the calling convention is soft-float.  We use the
> existing argument lowering hooks to avoid generating nodes with
> illegal types, and instead generate target-specific nodes:
> ARMISD::VMOVRRD and ARMISD::VMOVDRR.  ARMISD::VMOVRRD takes two i32
> values and produces an f64, and ARMISD::VMOVDRR takes an f64 and
> produces two i32 values.  This is implemented in
> ARMTargetLowering::LowerReturn,
> ARMTargetLowering::LowerFormalArguments, and
> ARMTargetLowering::LowerCall.  (See
> ARMTargetLowering::PassF64ArgInRegs etc.)
> 
> As far as I can tell, the SPE target is similar, so a similar
> approach should fix the issue, without any changes to
> target-independent code.
> 
> It would be possible to add some target-independent code to make this 
> sort of handling a little easier.  We could define target-independent 
> nodes which are equivalent to the ARMISD nodes I mentioned.  And if
> we had those nodes, we could add a target-independent helper for
> argument lowering that would generate them.  Not sure how much of
> that is actually worth doing; probably a lot of work to remove a
> small amount of redundant code.
> 
> -Eli
> 
> On 1/4/2019 6:05 AM, Nemanja Ivanovic wrote:
> > + Eli Friedman as he often has very insightful comments regarding
> > back end changes.
> >
> > On Fri, Jan 4, 2019 at 9:03 AM Nemanja Ivanovic 
> > <nemanja.i.ibm at gmail.com <mailto:nemanja.i.ibm at
gmail.com>> wrote:
> >
> >     The changes seem fine to me. I don't think this is excessively
> >     intrusive and it accomplishes what is needed by targets whose
> > call lowering can introduce illegal types.
> >     Adding Justin Bogner as the owner of SDAG and Hal Finkel as the
> >     PPC back end owner for their opinions.
> >
> >     Nemanja
> >
> >     On Thu, Jan 3, 2019 at 4:54 PM Justin Hibbits
> >     <jrh29 at alumni.cwru.edu <mailto:jrh29 at
alumni.cwru.edu>> wrote:
> >
> >         Hi Nemanja,
> >
> >         I'm attaching a patch that builds on D54583 and implements
> > what we discussed on IRC earlier today.  Particularly:
> >
> >         * Make LowerCallTo() a virtual function, so it can be
> > wrapped by a subclass.
> >         * Implement LowerCallTo() in PPCTargetLowering to wrap
> >           TargetLowering::LowerCallTo() and legalize the return
> > node when targeting SPE.
> >         * Augment PPCTargetLowering::LowerCall_32SVR4() to legalize
> >         MVT::f64
> >           arguments that have been pre-processed into
> >           EXTRACT_ELEMENT(i64 BITCAST f64, 0/1)
> >
> >         The purpose of this being to legalize intermediate illegal
> > types post-type legalization.
> >
> >         Is there a better approach?  Comments from anyone else?
> >
> >         - Justin
> >
> >         On Wed, 2 Jan 2019 11:39:59 -0500
> >         Nemanja Ivanovic <nemanja.i.ibm at gmail.com
> >         <mailto:nemanja.i.ibm at gmail.com>> wrote:
> >  
> >         > It sounds like the legalizer is lowering `fmaxnum` to a
> >         > libcall because it is not a legal node for `f64` and in
> >         > doing so, it is producing the `build_pair` to reassemble
> >         > the results of the  
> >         libcall.  
> >         > And presumably, it is assuming that the new nodes do not
> >         > need legalization or something along those lines.
> >         >
> >         > Justin, it would probably be good if you could provide
> >         > the  
> >         DAG before  
> >         > and after legalization both with and without your patch.
> >         Then we can  
> >         > see how it was handled before your patch and how it is  
> >         handled now  
> >         > and the difference should allude to the problem.
> >         >
> >         > N
> >         >
> >         > On Wed, Jan 2, 2019 at 10:54 AM Justin Hibbits via
> >         > llvm-dev < llvm-dev at lists.llvm.org
> >         > <mailto:llvm-dev at lists.llvm.org>> wrote: 
> >         > > Hi,
> >         > >
> >         > > I have a custom lowering operation on ISD::BITCAST
for
> >         > > the PowerPC/SPE target, to convert 'f64 bitcast
(i64
> >         build_pair i32,  
> >         > > i32)' into a 'f64 BUILD_SPE64 i32, i32'
node, which can
> >         > > be  
> >         seen at  
> >         > > https://reviews.llvm.org/D54583. However, when
building
> >         > > compiler-rt's lib/builtins/divdc3.c an assertion
is
> >         triggered that  
> >         > > BUILD_PAIR is not legal on line 24.  There should be
no
> >         > > bitcast(buildpair) anywhere in the generation, as it
> >         should all be  
> >         > > lowered.  However, this is not the case when
lowering
> >         > > to a  
> >         libcall  
> >         > > it seems.  The relevant debug output, is here:
> >         > >
> >         > > Creating new node: t118: i64 = build_pair t117,t116,
> >         > >  
> >        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
> >         > > Creating new node: t119: f64 = bitcast t118,
> >         > >  
> >        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
> >         > > Created libcall: t119: f64 = bitcast t118,
> >         > >  
> >        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
> >         > > Successfully converted node to libcall
> >         > >  ... replacing: t38: f64 = fmaxnum t36, t37,
> >         > >  
> >        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
> >         > >      with:      t119: f64 = bitcast t118,
> >         > >  
> >        
/home/chmeee/freebsd/contrib/compiler-rt/lib/builtins/divdc3.c:24:22
> >         > >
> >         > > Is this a real bug, or am I missing something in my
> >         patch?  After  
> >         > > spending quite a while on it I'm at a loss.
> >         > >
> >         > > Thanks,
> >         > > Justin
> >         > > _______________________________________________
> >         > > LLVM Developers mailing list
> >         > > llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
> >         > >
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >         > >  
> >  
>