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

Finkel, Hal J. via llvm-dev llvm-dev at lists.llvm.org
Fri Jan 4 10:06:21 PST 2019

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 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::LowerCall32SVR4() to legalize MVT::f64 arguments that have been pre-processed into EXTRACTELEMENT(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 buildpair 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 buildpair i32, i32)' into a 'f64 BUILDSPE64 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 BUILDPAIR 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 = buildpair 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

-- Hal Finkel Lead, Compiler Technology and Programming Languages Leadership Computing Facility Argonne National Laboratory

More information about the llvm-dev mailing list