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

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

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

On Wed, 2 Jan 2019 at 15:54, 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.
Hi Justin,

I don't think this is a bug in ConvertNodeToLibCall - rather, it's
just a case where you can be tripped up by the very late lowering of
intrinsics calls. This takes place post-legalisation. The issue isn't
the build_pair, it's the fact that you're constructing an i64 prior to
bitcasting to f64, and i64 is not legal on your target. Because these
bitcasts can be introduced so late, a custom bitcast lowering strategy
won't work.

It might be helpful to look at how I handle passing f64 on RISC-V. See
BuildPairF32 and SplitF64 in lib/Target/RISCV. I can't guarantee this
is the _best_ way of handling this sort of problem, but I did explore
quite a lot of options including a similar approach to the custom
bitcast lowering strategy you use here. I'm following this thread
closely to see if there are suggestions for better ways of handling
this issue. The custom inserter shouldn't be necessary in your case -
for RV32FD we don't have an instruction to directly transfer from a
64-bit FPR to the 32-bit GPRs.

Best,

Alex

On Wed, 2 Jan 2019 at 16:41, Alex Bradbury <asb at lowrisc.org>
wrote:
>
> On Wed, 2 Jan 2019 at 15:54, 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.
>
> Hi Justin,
>
> I don't think this is a bug in ConvertNodeToLibCall - rather, it's
> just a case where you can be tripped up by the very late lowering of
> intrinsics calls.
Sorry, I was imprecise. This isn't a general problem with lowering to
intrinsics / libcalls, just where a SelectionDAG node is expanded to a
libcall.

Best,

Alex

Hi Nemanja,

The behavior is the same prior to my patch and with my patch, with
regard to this specific behavior.  The custom BITCAST lowerer doesn't
introduce any change with this.  In fact, it was reported on
https://reviews.llvm.org/D49754 as behavior introduced with the
original SPE additions, so is inherent in this target from the
beginning.  So, it seems to me that the new nodes added in the libcall
lowering should themselves be lowered, unless I should do as Alex
alluded to, and add new legal nodes that themselves handle the argument
splitting and reforming.  But it seems to me that the machinery to do
that already exists, but might need another pass?

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

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>