llvm dev - Jun 2019 - Bug: Library functions for ISD::SRA, ISD::SHL, and ISD::SRL

LLVM appears to support Library functions for ISD::SRA ,ISD::SHL, and ISD::SRL,
as they are properly defined in RuntimeLibCalls.def.

The library functions defined in RuntimeLibCalls.def (among others) are these:

HANDLE_LIBCALL(SRA_I16, "__ashrhi3")
HANDLE_LIBCALL(SRA_I32, "__ashrsi3")
HANDLE_LIBCALL(SRA_I64, "__ashrdi3")

However, setting

setOperationAction(ISD::SRA, MVT::i16, LibCall);

in the TargetLowering constructor causes LLVM to stop with an assert as the
shift instruction can’t be selected.

The problem is in SelectionDAGLegalize::ConvertNodeToLibcall because there’s no
switch case for any of the Shift instructions.

The problem gets solved by just adding switch cases like this (and similar for
the other shift instructions):

case ISD::SRA:
      Results.push_back(ExpandIntLibCall(Node, false,
                                       RTLIB::SRA_I16,
                                       RTLIB::SRA_I16, RTLIB::SRA_I32,
                                       RTLIB::SRA_I64, RTLIB::SRA_I128));

I think this is a BUG by omission of necessary switch cases.

 
Similarly, the following ISD codes  ISD::CTTZ, ISD::CTLZ, ISD::CTPOP do not
define any Library calls, despite LLVM being able to fully expand them into
rather large code for targets that do not natively implement them.
I regard this also as an omission/bug, because not all targets would benefit
from the custom expansion of these ISD codes, which as said can get
unnecessarily long and costly.
Instead, LLVM should have them available as possible LibCalls.

Any comments or opinions on these subjects are appreciated

Joan Lluch

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190610/38ae307b/attachment.html>

All in-tree targets have variable shift instructions for native integer types,
except for AVR.  And AVR implements custom lowering.  I’m not sure what else
would be required to actually make marking a shift as “libcall” actually work
well; technically, the change you’re proposing might produce valid code, but it
would be terrible quality in a lot of cases.  So I’m not eager to add partial
support just for out-of-tree targets.

Marking CTLZ_ZERO_UNDEF as “LibCall” was implemented in
https://reviews.llvm.org/D47917 .  Probably straightforward to extend that to
cover CTTZ_ZERO_UNDEF and CTPOP.

-Eli

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Joan
Lluch via llvm-dev
Sent: Monday, June 10, 2019 8:31 AM
To: llvm-dev <llvm-dev at lists.llvm.org>
Subject: [EXT] [llvm-dev] Bug: Library functions for ISD::SRA, ISD::SHL, and
ISD::SRL

LLVM appears to support Library functions for ISD::SRA ,ISD::SHL, and ISD::SRL,
as they are properly defined in RuntimeLibCalls.def.

The library functions defined in RuntimeLibCalls.def (among others) are these:

HANDLE_LIBCALL(SRA_I16, "__ashrhi3")
HANDLE_LIBCALL(SRA_I32, "__ashrsi3")
HANDLE_LIBCALL(SRA_I64, "__ashrdi3")

However, setting

setOperationAction(ISD::SRA, MVT::i16, LibCall);

in the TargetLowering constructor causes LLVM to stop with an assert as the
shift instruction can’t be selected.

The problem is in SelectionDAGLegalize::ConvertNodeToLibcall because there’s no
switch case for any of the Shift instructions.

The problem gets solved by just adding switch cases like this (and similar for
the other shift instructions):

case ISD::SRA:
      Results.push_back(ExpandIntLibCall(Node, false,
                                       RTLIB::SRA_I16,
                                       RTLIB::SRA_I16, RTLIB::SRA_I32,
                                       RTLIB::SRA_I64, RTLIB::SRA_I128));

I think this is a BUG by omission of necessary switch cases.


Similarly, the following ISD codes  ISD::CTTZ, ISD::CTLZ, ISD::CTPOP do not
define any Library calls, despite LLVM being able to fully expand them into
rather large code for targets that do not natively implement them.
I regard this also as an omission/bug, because not all targets would benefit
from the custom expansion of these ISD codes, which as said can get
unnecessarily long and costly.
Instead, LLVM should have them available as possible LibCalls.

Any comments or opinions on these subjects are appreciated

Joan Lluch


-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190610/cef3869f/attachment.html>

Hi Eli,

Thanks for pointing to the CTLZ_ZERO_UNDEF “LibCall” implementation. I have not
it in the version that I am currently using, so it’s nice to know that it’s
implemented now.

Incidentally, the CTLZ… implementation is IDENTICAL to what I am proposing for
the Shifts. This is not just adding support for “out-of-tree-targets”, but
giving consistency to the fact that we have perfectly defined LibCalls for
Shifts, but they can’t be used because there’s an omission in the sources.

I only proposed to add a missing case statement to the ConvertNodeToLibCall.

To my understanding this is totally harmless to any existing or future targets
because all what it does is to call the Library function when setOperationAction
is set to “LibCall”. This is not different that any other case, and you just
confirmed that you added the same for the CTLZ… functions.  So I do not
understand what you mean by “terrible quality in a lot of cases”:  Targets with
native shift support are not affected, and neither are targets with custom
lowering.

I would suggest that you look in more detail at the actual implementation of the
ConvertNodeToLibCall and from where it is called and its actual purpose. There’s
nothing else required. So I would appreciate that you elaborate on why you think
that this is not a good idea.

Thanks,

John Lluch

> On 10 Jun 2019, at 21:09, Eli Friedman <efriedma at quicinc.com>
wrote:
> 
> All in-tree targets have variable shift instructions for native integer
types, except for AVR.  And AVR implements custom lowering.  I’m not sure what
else would be required to actually make marking a shift as “libcall” actually
work well; technically, the change you’re proposing might produce valid code,
but it would be terrible quality in a lot of cases.  So I’m not eager to add
partial support just for out-of-tree targets.
>  
> Marking CTLZ_ZERO_UNDEF as “LibCall” was implemented in
https://reviews.llvm.org/D47917 <https://reviews.llvm.org/D47917> . 
Probably straightforward to extend that to cover CTTZ_ZERO_UNDEF and CTPOP.
>  
> -Eli
>  
> From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Joan
Lluch via llvm-dev
> Sent: Monday, June 10, 2019 8:31 AM
> To: llvm-dev <llvm-dev at lists.llvm.org>
> Subject: [EXT] [llvm-dev] Bug: Library functions for ISD::SRA, ISD::SHL,
and ISD::SRL
>  
> LLVM appears to support Library functions for ISD::SRA ,ISD::SHL, and
ISD::SRL, as they are properly defined in RuntimeLibCalls.def.
>  
> The library functions defined in RuntimeLibCalls.def (among others) are
these:
>  
> HANDLE_LIBCALL(SRA_I16, "__ashrhi3")
> HANDLE_LIBCALL(SRA_I32, "__ashrsi3")
> HANDLE_LIBCALL(SRA_I64, "__ashrdi3")
>  
> However, setting
>  
> setOperationAction(ISD::SRA, MVT::i16, LibCall);
>  
> in the TargetLowering constructor causes LLVM to stop with an assert as the
shift instruction can’t be selected.
>  
> The problem is in SelectionDAGLegalize::ConvertNodeToLibcall because
there’s no switch case for any of the Shift instructions.
>  
> The problem gets solved by just adding switch cases like this (and similar
for the other shift instructions):
>  
> case ISD::SRA:
>       Results.push_back(ExpandIntLibCall(Node, false,
>                                        RTLIB::SRA_I16,
>                                        RTLIB::SRA_I16, RTLIB::SRA_I32,
>                                        RTLIB::SRA_I64, RTLIB::SRA_I128));
>  
> I think this is a BUG by omission of necessary switch cases.
>  
>  
> Similarly, the following ISD codes  ISD::CTTZ, ISD::CTLZ, ISD::CTPOP do not
define any Library calls, despite LLVM being able to fully expand them into
rather large code for targets that do not natively implement them.
> I regard this also as an omission/bug, because not all targets would
benefit from the custom expansion of these ISD codes, which as said can get
unnecessarily long and costly.
> Instead, LLVM should have them available as possible LibCalls.
>  
> Any comments or opinions on these subjects are appreciated
>  
> Joan Lluch
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190611/d7902e68/attachment.html>