<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>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?</p>
<p> -Hal<br>
</p>
<div class="moz-cite-prefix">On 1/4/19 8:03 AM, Nemanja Ivanovic wrote:<br>
</div>
<blockquote type="cite" cite="mid:CAObEeNg+b+CqwzB_hO9CQ8kKmDpjpeuM9_U1ztzYhKdt22xSVQ@mail.gmail.com">
<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" 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>
<pre class="moz-signature" cols="72">--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory</pre>
</body>
</html>