[llvm-dev] Potential bug in SelectionDAGLegalize::ConvertNodeToLibcall()?
Justin Hibbits via llvm-dev
llvm-dev at lists.llvm.org
Wed Jan 9 07:54:54 PST 2019
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
> > > >
> >
>
More information about the llvm-dev
mailing list