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

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 --------------
A non-text attachment was scrubbed...
Name: llvm_lowercall.diff
Type: text/x-patch
Size: 2849 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190103/e0e3caaa/attachment.bin>

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/d7164eb2/attachment.html>

+ 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>

Aside from the fact that you're checking for i64 specifically instead of
generally checking for illegal types, how much of this is really PPC specific?
Would this be a reasonable enhancement to the SDAG logic in general?

 -Hal

On 1/4/19 8:03 AM, Nemanja Ivanovic 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
> >

--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190104/4bfe4ee5/attachment.html>