Re: [PATCH v1 2/2] RISC-V: Adjust predicates for immediate shift operands

[Philipp Tomsich <philipp.tomsich@vrull.eu>]

Jim,

On Mon, 16 Nov 2020 at 23:28, Jim Wilson <jimw@sifive.com> wrote:
>
> On Mon, Nov 16, 2020 at 10:57 AM Philipp Tomsich <philipp.tomsich@vrull.eu>
wrote:
>>
>> In case a negative shift operand makes it through into the backend,
>> it will be treated as unsigned and truncated (using a mask) to fit
>> into the range 0..31 (for SImode) and 0..63 (for DImode).
>
>
> This is a de-optimization.  This doesn't make any sense.  The ISA manual
clearly states the shift counts are truncated.  Some targets do this with
SHIFT_COUNT_TRUNCATED, but that is known to cause problems, so the RISC-V
port is doing it in the shift expanders.  I believe that other targets do
this too.

This is an de-optimization only, if applied without patch 1 from the
series: the change to VRP ensures that the backend will never see a shift
wider than the immediate field.
The problem is that if a negative shift-amount makes it to the backend,
unindented code may be generated (as a shift-amount, in my reading, should
always be interpreted as unsigned).

Note that with tree-vrp turned on (and patch 1 of the series applied), you
will see

.L3:
li a0,0

generated, anyway.

> Also, note that replacing
>   slli a0, a0, 31
> with
>   li a1, -1;
>   sll a0, a0, a1
> doesn't change the operation performed.  The shift count is still
truncated to 31, and so you get the exact same result from both code
sequences.  All you have done is make the code bigger and slower which is
undesirable.

I do agree that this does not address the issue of a shift that is wider
than the register width, even though it makes sure we reject this from the
immediate field.
That said: what is the correct behavior/result of this operation?

> Also note that the testcase has implementation defined results, so there
is no wrong answer here, and nothing wrong with what the RISC-V port is
doing.
>
>> +/* { dg-final { scan-assembler "sll" } } */
>
>
> I don't think that this will work as a grep for sll will also match
slli.  You would need to add a space or tab or maybe both to the search
string to prevent matches with slli.  Or alternatively use
scan-assembler-not "slli" which will match and fail for both slli and slliw.

Good catch. I turned this check around, but submitted the wrong one.

Thanks,
Philipp.