Re: [RFC] SHIFT_COUNT_TRUNCATED and shift_truncation_mask

[Jeff Law <law at redhat dot com>]

On 5/10/19 1:08 AM, Richard Biener wrote:
> On Thu, May 9, 2019 at 6:00 PM Jeff Law <law@redhat.com> wrote:
>>
>> On 5/9/19 5:52 AM, Robin Dapp wrote:
>>> Hi,
>>>
>>> while trying to improve s390 code generation for rotate and shift I
>>> noticed superfluous subregs for shift count operands. In our backend we
>>> already have quite cumbersome patterns that would need to be duplicated
>>> (or complicated further by more subst patterns) in order to get rid of
>>> the subregs.
>>>
>>> I had already finished all the patterns when I realized that
>>> SHIFT_COUNT_TRUNCATED and the target hook shift_truncation_mask already
>>> exist and could do what is needed without extra patterns.  Just defining
>>>  shift_truncation_mask was not enough though as most of the additional
>>> insns get introduced by combine.
>>>
>>> Event SHIFT_COUNT_TRUNCATED is no perfect match to what our hardware
>>> does because we always only consider the last 6 bits of a shift operand.>
>>> Despite all the warnings in the other backends, most notably
>>> SHIFT_COUNT_TRUNCATED being "discouraged" as mentioned in riscv.h, I
>>> wrote the attached tentative patch.  It's a little ad-hoc, uses the
>>> SHIFT_COUNT_TRUNCATED paths only if shift_truncation_mask () != 0 and,
>>> instead of truncating via & (GET_MODE_BITSIZE (mode) - 1), it applies
>>> the mask returned by shift_truncation_mask.  Doing so, usage of both
>>> "methods" actually reduces to a single way.
>> THe main reason it's discouraged is because some targets have insns
>> where the count would be truncated and others where it would not.   So
>> for example normal shifts might truncate, but vector shifts might or
>> (mips) or shifts might truncate but bit tests do not (x86).
>>
>> I don't know enough about the s390 architecture to know if there's any
>> corner cases.  You'd have to look at ever pattern in your machine
>> description with a shift and verify that it's going to DTRT if the count
>> hasn't been truncated.
>>
>>
>> It would really help if you could provide testcases which show the
>> suboptimal code and any analysis you've done.
> 
> The main reason I dislike SHIFT_COUNT_TRUNCATED is that it
> changes the meaning of the IL.  We generally want these kind
> of things to be explicit.
Understood and agreed.  So I think this argues that Robin should look at
a different approach and we should start pushing ports away from
SHIFT_COUNT_TRUNCATED a bit more aggressively.

Jeff