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