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

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::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

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



More information about the llvm-dev mailing list