Re: [PATCH] riscv: implement TARGET_MODE_REP_EXTENDED

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

[-- Attachment #1: Type: text/plain, Size: 3889 bytes --]

Alexander,

I had missed your comment until now.

On Tue, 6 Sept 2022 at 13:39, Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Mon, 5 Sep 2022, Philipp Tomsich wrote:
>
> > +riscv_mode_rep_extended (scalar_int_mode mode, scalar_int_mode
mode_rep)
> > +{
> > +  /* On 64-bit targets, SImode register values are sign-extended to
DImode.  */
> > +  if (TARGET_64BIT && mode == SImode && mode_rep == DImode)
> > +    return SIGN_EXTEND;
>
> I think this leads to a counter-intuitive requirement that a hand-written
> inline asm must sign-extend its output operands that are bound to either
> signed or unsigned 32-bit lvalues. Will compiler users be aware of that?

I am not sure if I fully understand your concern, as the mode of the
asm-output will be derived from the variable type.
So "asm (... : "=r" (a))" will take DI/SI/HI/QImode depending on the type
of a.

The concern, as far as I understand would be the case where the
assembly-sequence leaves an incompatible extension in the register.
So l understand the problem being:
    int a;
    asm volatile ("zext.w %0, %1" : "=r"(a) : "r"(b));  // possible pitfall
-- the programmer wants a SImode representation (so it needs to be extended)
but not
    long long a;
    asm volatile ("zext.w %0, %1" : "=r"(a): "r"(b));   // possible pitfall
-- the programmer wants the DImode representation (don't extend)

If we look at the output of expand for the first case (I made sure to
consume "a" again using a "asm volatile ("nop" : : "r"(a))"), we see that
an explicit extension is created from SImode to DImode (the "(subreg:SI
(reg:DI ...) 0)" in the asm-operand for the next block is a bigger concern
— this may be an artifact of TARGET_TRULY_NOOP_TRUNCATION not being
properly defined on RISC-V; but this issue didn't originate from this
patch):

;; __asm__ __volatile__("zext.w %0,%1" : "=r" a_2 : "r" b_1(D));
(insn 7 5 6 (set (reg:SI 75 [ aD.1490 ])
        (asm_operands/v:SI ("zext.w %0,%1") ("=r") 0 [
                (reg/v:DI 74 [ bD.1487 ])
            ]
             [
                (asm_input:DI ("r") ./rep-asm.c:5)
            ]
             [] ./rep-asm.c:5)) "./rep-asm.c":5:3 -1
     (nil))
(insn 6 7 0 (set (reg/v:DI 72 [ aD.1490 ])
        (sign_extend:DI (reg:SI 75 [ aD.1490 ]))) "./rep-asm.c":5:3 -1
     (nil))
;; __asm__ __volatile__("nop" :  : "r" a_2);
(insn 8 6 0 (asm_operands/v ("nop") ("") 0 [
            (subreg/s/u:SI (reg/v:DI 72 [ aD.1490 ]) 0)
        ]
         [
            (asm_input:SI ("r") ./rep-asm.c:6)
        ]
         [] ./rep-asm.c:6) "./rep-asm.c":6:3 -1
     (nil))

which translates to:

f:

#APP

# 5 "./rep-asm.c" 1

zext.w a0,a0

# 0 "" 2

#NO_APP

sext.w a0,a0

#APP

# 6 "./rep-asm.c" 1

nop

# 0 "" 2

#NO_APP


If this patch is not applied (and TARGET_MODE_REP_EXTENDED is not defined),
we see the sext.w missing.
So in fact, we may have unintentionally fixed an issue?

> Moreover, without adjusting TARGET_TRULY_NOOP_TRUNCATION this should cause
> miscompilation when a 64-bit variable is truncated to 32 bits: the
pre-existing
> hook says that nothing needs to be done to truncate, but the new hook says
> that the result of the truncation is properly sign-extended.
>
> The documentation for TARGET_MODE_REP_EXTENDED warns about that:
>
>     In order to enforce the representation of mode,
TARGET_TRULY_NOOP_TRUNCATION
>     should return false when truncating to mode.

Good point.  This should indeed be added and we'll look into this in more
detail for v2.

Looking into this in more detail, it seems that this change doesn't make
things worse (we already had that problem before, as an explicit extension
might also lead to the same reasoning).
So somehow we've been avoiding this bullet so far, although I don't know
yet how...

Philipp.