[RISCV] [GNU AS] Possible `vmsge{u}.vx` instruction lowering bug?

[RISCV] [GNU AS] Possible `vmsge{u}.vx` instruction lowering bug?

[im Kiva]

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

Hi maintainers,

I discovered that GNU Assembler (as) lowers `vmsge.vx` and `vmsgeu.vx` (pseudo
instructions from RISC-V Vector Extension [1]) when the destination
register is v0 as follows:

vmsge{u}.vx v0, v4, a0, v0.t, v2

will be expanded to:

vmslt{u}.vx v2, v4, a0, v0.t
vmandn.mm v0, v0, v2

You can inspect the lowering result with Godbolt [2].
However, according to the Vector specification [1] page 52. The "desugared"
`vmslt{u}.vx` is not masked:

> masked va >= x, vd == v0
>   pseudoinstruction: vmsge{u}.vx vd, va, x, v0.t, vt
>   expansion: vmslt{u}.vx vt, va, x; vmandn.mm vd, vd, vt

So the spec-expected result of the example above should be:
vmslt{u}.vx v2, v4, a0 <-- no v0.t here
vmandn.mm v0, v0, v2

I thus submitted a patch to the LLVM [3], and it was accepted recently.

I am wondering if binutils considers it a bug, or if it is just intentional
because of some historical and compatibility reasons.
Thanks.

Best regards
Kiva Oyama

[1]: https://github.com/riscv/riscv-v-spec/releases/tag/v1.0
[2]: https://godbolt.org/z/aszc5d8sh
[3]: https://reviews.llvm.org/D158392

Re: [RISCV] [GNU AS] Possible `vmsge{u}.vx` instruction lowering bug?

[Jan Beulich]

On 29.08.2023 08:52, im Kiva via Binutils wrote:
> Hi maintainers,
> 
> I discovered that GNU Assembler (as) lowers `vmsge.vx` and `vmsgeu.vx` (pseudo
> instructions from RISC-V Vector Extension [1]) when the destination
> register is v0 as follows:
> 
> vmsge{u}.vx v0, v4, a0, v0.t, v2
> 
> will be expanded to:
> 
> vmslt{u}.vx v2, v4, a0, v0.t
> vmandn.mm v0, v0, v2
> 
> You can inspect the lowering result with Godbolt [2].
> However, according to the Vector specification [1] page 52. The "desugared"
> `vmslt{u}.vx` is not masked:
> 
>> masked va >= x, vd == v0
>>   pseudoinstruction: vmsge{u}.vx vd, va, x, v0.t, vt
>>   expansion: vmslt{u}.vx vt, va, x; vmandn.mm vd, vd, vt
> 
> So the spec-expected result of the example above should be:
> vmslt{u}.vx v2, v4, a0 <-- no v0.t here
> vmandn.mm v0, v0, v2
> 
> I thus submitted a patch to the LLVM [3], and it was accepted recently.
> 
> I am wondering if binutils considers it a bug, or if it is just intentional
> because of some historical and compatibility reasons.

I think this wants fixing alike in binutils: From looking at vector_macro(),
it appears that emitting the masked form is merely an accident resulting
from the inverted encoding of "masking". In particular, if masking was
indeed meant, I expect code there would be

		  if (vd == vm)
		    {
		      macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp, vs2, vs1, vm);
		      macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vm, vtemp);
		    }
		  else
		    ...

much like it is a few lines down from there. (Apparently the "else" path
omitted above is similarly affected.)

Jan

Re: [RISCV] [GNU AS] Possible `vmsge{u}.vx` instruction lowering bug?

[Nelson Chu]

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

Is the following expected?  Seems "vd is any" also has the same issue.

Nelson

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c

index 959cbbc32a5..e49b34fd524 100644

--- a/gas/config/tc-riscv.c

+++ b/gas/config/tc-riscv.c

@@ -1966,13 +1966,13 @@ vector_macro (struct riscv_cl_insn *ip)

          /* Masked.  Have vtemp to avoid overlap constraints.  */

          if (vd == vm)

            {

-             macro_build (NULL, "vmslt.vx", "Vd,Vt,s", vtemp, vs2, vs1);

+             macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp, vs2, vs1,
-1);

              macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vm, vtemp);

            }

          else

            {

              /* Preserve the value of vd if not updating by vm.  */

-             macro_build (NULL, "vmslt.vx", "Vd,Vt,s", vtemp, vs2, vs1);

+             macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp, vs2, vs1,
-1);

              macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vtemp, vm,
vtemp);

              macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vd, vm);

              macro_build (NULL, "vmor.mm", "Vd,Vt,Vs", vd, vtemp, vd);

@@ -2001,13 +2001,13 @@ vector_macro (struct riscv_cl_insn *ip)

          /* Masked.  Have vtemp to avoid overlap constraints.  */

          if (vd == vm)

            {

-             macro_build (NULL, "vmsltu.vx", "Vd,Vt,s", vtemp, vs2, vs1);

+             macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vtemp, vs2, vs1,
-1);

              macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vm, vtemp);

            }

          else

            {

              /* Preserve the value of vd if not updating by vm.  */

-             macro_build (NULL, "vmsltu.vx", "Vd,Vt,s", vtemp, vs2, vs1);

+             macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vtemp, vs2, vs1,
-1);

              macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vtemp, vm,
vtemp);

              macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vd, vm);

              macro_build (NULL, "vmor.mm", "Vd,Vt,Vs", vd, vtemp, vd);

diff --git a/gas/testsuite/gas/riscv/vector-insns-vmsgtvx.d
b/gas/testsuite/gas/riscv/vector-insns-vmsgtvx.d

index dcc951a3cbf..aa633e357c9 100644

--- a/gas/testsuite/gas/riscv/vector-insns-vmsgtvx.d

+++ b/gas/testsuite/gas/riscv/vector-insns-vmsgtvx.d

@@ -11,9 +11,9 @@ Disassembly of section .text:

 [      ]+[0-9a-f]+:[   ]+76422257[     ]+vmnot.m[      ]+v4,v4

 [      ]+[0-9a-f]+:[   ]+6cc64457[     ]+vmslt.vx[     ]+v8,v12,a2,v0.t

 [      ]+[0-9a-f]+:[   ]+6e802457[     ]+vmxor.mm[     ]+v8,v8,v0

-[      ]+[0-9a-f]+:[   ]+6c85c657[     ]+vmslt.vx[     ]+v12,v8,a1,v0.t

+[      ]+[0-9a-f]+:[   ]+6e85c657[     ]+vmslt.vx[     ]+v12,v8,a1

 [      ]+[0-9a-f]+:[   ]+62062057[     ]+vmandn.mm[    ]+v0,v0,v12

-[      ]+[0-9a-f]+:[   ]+6c85c657[     ]+vmslt.vx[     ]+v12,v8,a1,v0.t

+[      ]+[0-9a-f]+:[   ]+6e85c657[     ]+vmslt.vx[     ]+v12,v8,a1

 [      ]+[0-9a-f]+:[   ]+62062657[     ]+vmandn.mm[    ]+v12,v0,v12

 [      ]+[0-9a-f]+:[   ]+62402257[     ]+vmandn.mm[    ]+v4,v4,v0

 [      ]+[0-9a-f]+:[   ]+6ac22257[     ]+vmor.mm[      ]+v4,v12,v4

@@ -21,9 +21,9 @@ Disassembly of section .text:

 [      ]+[0-9a-f]+:[   ]+76422257[     ]+vmnot.m[      ]+v4,v4

 [      ]+[0-9a-f]+:[   ]+68c64457[     ]+vmsltu.vx[    ]+v8,v12,a2,v0.t

 [      ]+[0-9a-f]+:[   ]+6e802457[     ]+vmxor.mm[     ]+v8,v8,v0

-[      ]+[0-9a-f]+:[   ]+6885c657[     ]+vmsltu.vx[    ]+v12,v8,a1,v0.t

+[      ]+[0-9a-f]+:[   ]+6a85c657[     ]+vmsltu.vx[    ]+v12,v8,a1

 [      ]+[0-9a-f]+:[   ]+62062057[     ]+vmandn.mm[    ]+v0,v0,v12

-[      ]+[0-9a-f]+:[   ]+6885c657[     ]+vmsltu.vx[    ]+v12,v8,a1,v0.t

+[      ]+[0-9a-f]+:[   ]+6a85c657[     ]+vmsltu.vx[    ]+v12,v8,a1

 [      ]+[0-9a-f]+:[   ]+62062657[     ]+vmandn.mm[    ]+v12,v0,v12

 [      ]+[0-9a-f]+:[   ]+62402257[     ]+vmandn.mm[    ]+v4,v4,v0

 [      ]+[0-9a-f]+:[   ]+6ac22257[     ]+vmor.mm[      ]+v4,v12,v4

On Tue, Aug 29, 2023 at 3:43 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 29.08.2023 08:52, im Kiva via Binutils wrote:
> > Hi maintainers,
> >
> > I discovered that GNU Assembler (as) lowers `vmsge.vx` and `vmsgeu.vx`
> (pseudo
> > instructions from RISC-V Vector Extension [1]) when the destination
> > register is v0 as follows:
> >
> > vmsge{u}.vx v0, v4, a0, v0.t, v2
> >
> > will be expanded to:
> >
> > vmslt{u}.vx v2, v4, a0, v0.t
> > vmandn.mm v0, v0, v2
> >
> > You can inspect the lowering result with Godbolt [2].
> > However, according to the Vector specification [1] page 52. The
> "desugared"
> > `vmslt{u}.vx` is not masked:
> >
> >> masked va >= x, vd == v0
> >>   pseudoinstruction: vmsge{u}.vx vd, va, x, v0.t, vt
> >>   expansion: vmslt{u}.vx vt, va, x; vmandn.mm vd, vd, vt
> >
> > So the spec-expected result of the example above should be:
> > vmslt{u}.vx v2, v4, a0 <-- no v0.t here
> > vmandn.mm v0, v0, v2
> >
> > I thus submitted a patch to the LLVM [3], and it was accepted recently.
> >
> > I am wondering if binutils considers it a bug, or if it is just
> intentional
> > because of some historical and compatibility reasons.
>
> I think this wants fixing alike in binutils: From looking at
> vector_macro(),
> it appears that emitting the masked form is merely an accident resulting
> from the inverted encoding of "masking". In particular, if masking was
> indeed meant, I expect code there would be
>
>                   if (vd == vm)
>                     {
>                       macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp,
> vs2, vs1, vm);
>                       macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd,
> vm, vtemp);
>                     }
>                   else
>                     ...
>
> much like it is a few lines down from there. (Apparently the "else" path
> omitted above is similarly affected.)
>
> Jan
>

Re: [RISCV] [GNU AS] Possible `vmsge{u}.vx` instruction lowering bug?

[Jan Beulich]

On 29.08.2023 09:59, Nelson Chu wrote:
> Is the following expected?

I think so, but Kiva - please confirm.

>  Seems "vd is any" also has the same issue.

Right, that's what ...

>> I think this wants fixing alike in binutils: From looking at
>> vector_macro(),
>> it appears that emitting the masked form is merely an accident resulting
>> from the inverted encoding of "masking". In particular, if masking was
>> indeed meant, I expect code there would be
>>
>>                   if (vd == vm)
>>                     {
>>                       macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp,
>> vs2, vs1, vm);
>>                       macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd,
>> vm, vtemp);
>>                     }
>>                   else
>>                     ...
>>
>> much like it is a few lines down from there. (Apparently the "else" path
>> omitted above is similarly affected.)

... I was referring to with the parenthesized sentence.

Jan

Re: [RISCV] [GNU AS] Possible `vmsge{u}.vx` instruction lowering bug?

[Nelson Chu]

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

Hi Jan,
Thanks for pointing that out.

Hi Kiva,
Maybe you can send a better patch for this issue if you are interested,
including what Jan suggested.  Thanks :-)

Nelson

On Tue, Aug 29, 2023 at 4:52 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 29.08.2023 09:59, Nelson Chu wrote:
> > Is the following expected?
>
> I think so, but Kiva - please confirm.
>
> >  Seems "vd is any" also has the same issue.
>
> Right, that's what ...
>
> >> I think this wants fixing alike in binutils: From looking at
> >> vector_macro(),
> >> it appears that emitting the masked form is merely an accident resulting
> >> from the inverted encoding of "masking". In particular, if masking was
> >> indeed meant, I expect code there would be
> >>
> >>                   if (vd == vm)
> >>                     {
> >>                       macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp,
> >> vs2, vs1, vm);
> >>                       macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd,
> >> vm, vtemp);
> >>                     }
> >>                   else
> >>                     ...
> >>
> >> much like it is a few lines down from there. (Apparently the "else" path
> >> omitted above is similarly affected.)
>
> ... I was referring to with the parenthesized sentence.
>
> Jan
>

Re: [RISCV] [GNU AS] Possible `vmsge{u}.vx` instruction lowering bug?

[Jan Beulich]

On 30.08.2023 04:51, Nelson Chu wrote:
> Hi Jan,
> Thanks for pointing that out.
> 
> Hi Kiva,
> Maybe you can send a better patch for this issue if you are interested,
> including what Jan suggested.  Thanks :-)

Hmm, both of your replies above make me suspect a misunderstanding: I
think your patch does exactly what is needed.

Jan

> On Tue, Aug 29, 2023 at 4:52 PM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> On 29.08.2023 09:59, Nelson Chu wrote:
>>> Is the following expected?
>>
>> I think so, but Kiva - please confirm.
>>
>>>  Seems "vd is any" also has the same issue.
>>
>> Right, that's what ...
>>
>>>> I think this wants fixing alike in binutils: From looking at
>>>> vector_macro(),
>>>> it appears that emitting the masked form is merely an accident resulting
>>>> from the inverted encoding of "masking". In particular, if masking was
>>>> indeed meant, I expect code there would be
>>>>
>>>>                   if (vd == vm)
>>>>                     {
>>>>                       macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp,
>>>> vs2, vs1, vm);
>>>>                       macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd,
>>>> vm, vtemp);
>>>>                     }
>>>>                   else
>>>>                     ...
>>>>
>>>> much like it is a few lines down from there. (Apparently the "else" path
>>>> omitted above is similarly affected.)
>>
>> ... I was referring to with the parenthesized sentence.
>>
>> Jan
>>
> 

Re: [RISCV] [GNU AS] Possible `vmsge{u}.vx` instruction lowering bug?

[Kiva Oyama]

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

>  Is the following expected?

Yes.

> Seems "vd is any" also has the same issue.

I rechecked the spec, for every masked vmsge{u}.vx, the lowered instruction
is only masked when `vd != v0`.
So your patch does fix these two problems.

Thank you for clarifying.

Best Regards,
Kiva Oyama

On Wed, Aug 30, 2023 at 1:51 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 30.08.2023 04:51, Nelson Chu wrote:
> > Hi Jan,
> > Thanks for pointing that out.
> >
> > Hi Kiva,
> > Maybe you can send a better patch for this issue if you are interested,
> > including what Jan suggested.  Thanks :-)
>
> Hmm, both of your replies above make me suspect a misunderstanding: I
> think your patch does exactly what is needed.
>
> Jan
>
> > On Tue, Aug 29, 2023 at 4:52 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> >> On 29.08.2023 09:59, Nelson Chu wrote:
> >>> Is the following expected?
> >>
> >> I think so, but Kiva - please confirm.
> >>
> >>>  Seems "vd is any" also has the same issue.
> >>
> >> Right, that's what ...
> >>
> >>>> I think this wants fixing alike in binutils: From looking at
> >>>> vector_macro(),
> >>>> it appears that emitting the masked form is merely an accident
> resulting
> >>>> from the inverted encoding of "masking". In particular, if masking was
> >>>> indeed meant, I expect code there would be
> >>>>
> >>>>                   if (vd == vm)
> >>>>                     {
> >>>>                       macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm",
> vtemp,
> >>>> vs2, vs1, vm);
> >>>>                       macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs",
> vd,
> >>>> vm, vtemp);
> >>>>                     }
> >>>>                   else
> >>>>                     ...
> >>>>
> >>>> much like it is a few lines down from there. (Apparently the "else"
> path
> >>>> omitted above is similarly affected.)
> >>
> >> ... I was referring to with the parenthesized sentence.
> >>
> >> Jan
> >>
> >
>
>

Re: [RISCV] [GNU AS] Possible `vmsge{u}.vx` instruction lowering bug?

[Nelson Chu]

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

Thanks for both your clarifications, committed.

Thanks
Nelson

On Fri, Sep 1, 2023 at 2:37 PM Kiva Oyama <libkernelpanic@gmail.com> wrote:

> >  Is the following expected?
>
> Yes.
>
> > Seems "vd is any" also has the same issue.
>
> I rechecked the spec, for every masked vmsge{u}.vx, the lowered
> instruction is only masked when `vd != v0`.
> So your patch does fix these two problems.
>
> Thank you for clarifying.
>
> Best Regards,
> Kiva Oyama
>
> On Wed, Aug 30, 2023 at 1:51 PM Jan Beulich <jbeulich@suse.com> wrote:
>
>> On 30.08.2023 04:51, Nelson Chu wrote:
>> > Hi Jan,
>> > Thanks for pointing that out.
>> >
>> > Hi Kiva,
>> > Maybe you can send a better patch for this issue if you are interested,
>> > including what Jan suggested.  Thanks :-)
>>
>> Hmm, both of your replies above make me suspect a misunderstanding: I
>> think your patch does exactly what is needed.
>>
>> Jan
>>
>> > On Tue, Aug 29, 2023 at 4:52 PM Jan Beulich <jbeulich@suse.com> wrote:
>> >
>> >> On 29.08.2023 09:59, Nelson Chu wrote:
>> >>> Is the following expected?
>> >>
>> >> I think so, but Kiva - please confirm.
>> >>
>> >>>  Seems "vd is any" also has the same issue.
>> >>
>> >> Right, that's what ...
>> >>
>> >>>> I think this wants fixing alike in binutils: From looking at
>> >>>> vector_macro(),
>> >>>> it appears that emitting the masked form is merely an accident
>> resulting
>> >>>> from the inverted encoding of "masking". In particular, if masking
>> was
>> >>>> indeed meant, I expect code there would be
>> >>>>
>> >>>>                   if (vd == vm)
>> >>>>                     {
>> >>>>                       macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm",
>> vtemp,
>> >>>> vs2, vs1, vm);
>> >>>>                       macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs",
>> vd,
>> >>>> vm, vtemp);
>> >>>>                     }
>> >>>>                   else
>> >>>>                     ...
>> >>>>
>> >>>> much like it is a few lines down from there. (Apparently the "else"
>> path
>> >>>> omitted above is similarly affected.)
>> >>
>> >> ... I was referring to with the parenthesized sentence.
>> >>
>> >> Jan
>> >>
>> >
>>
>>