[llvm-dev] Potential bug in SelectionDAGLegalize::ConvertNodeToLibcall()?

Fri Jan 4 14:35:51 PST 2019

On Fri, 4 Jan 2019 18:06:21 +0000
"Finkel, Hal J." <hfinkel at anl.gov> wrote:

> On 1/4/19 11:22 AM, Justin Hibbits wrote:
> > It's really not at all PPC specific.  The goal is to legalize DAGs
> > that have an intermediate illegal type to satisfy the calling
> > convention.  I don't know if other ABIs have this, but the reason
> > this is an issue for SPE is that it uses 64-bit doubles in
> > registers, but passes these around in 32-bit GPR pairs, so
> > necessitates having an intermediate illegal i64 type.  If this is
> > specific to PPC, then it might make more sense to stay PPC-specific
> > with the trivial virtual override.  But if other ABIs have this
> > requirement (does ARM EABI have this?) then pushing it into the
> > general SDAG logic is probably better.  It really just would mean
> > adding a legalizing pass over the call and return nodes
> > immediately, or pushing it up to the Libcall logic, which started
> > this thread in the first place.  
> 
> 
> Thanks. I'm wondering, in particular, whether adding a generalized
> version of the logic in your LowerCallTo override into the base
> version would be meaningful / cause behavioral changes for existing
> targets?
> 
>  -Hal
> 

Justin Bogner  would know better.  I think it would probably be better
in the base version, but don't know what the best way to implement it
would be.

- Justin

> 
> >
> > - Justin
> >
> > On Fri, 4 Jan 2019 17:00:49 +0000
> > "Finkel, Hal J." <hfinkel at anl.gov> wrote:
> >  
> >> 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  
> 