Re: Re: RISC-V: Support XTheadVector extensions

["Christoph Müllner" <christoph.muellner@vrull.eu>]

On Wed, Nov 22, 2023 at 2:52 PM 钟居哲 <juzhe.zhong@rivai.ai> wrote:
>
> I am totally ok to approve theadvector on GCC-14 before stage 3 close
> as long as it doesn't touch the current RVV codes too much and binutils supports theadvector.
>
> I have provided the draft approach:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637349.html
> which turns out doesn't need to change any codes of vector.md.
> I strongly suggest follow this draft. I can be actively review theadvector during stage 3.
> And hopefully can help you land theadvector on GCC-14.

I see now two approaches:
1) Let GCC emit RVV instructions for XTheadVector for instructions
that are in both
2) Use the ASM_OUTPUT_OPCODE hook to output "th." for these instructions

No doubt, the ASM_OUTPUT_OPCODE hook approach is better than
our format-string approach, but would 1) not be the even better solution?
It would also mean, that not a single test case is required for these
overlapping instructions (only a few tests that ensure that we don't emit
RVV instructions that are not available in XTheadVector).
Besides that, letting GCC emit RVV instructions for XTheadVector is a
very clever idea,
because it fully utilizes the fact that both extensions overlap to a
huge degree.

The ASM_OUTPUT_OPCODE approach could lead to an issue if we enable XTheadVector
with any other vector extension, say Zvfoo. In this case the Zvfoo
instructions will
all be prefixed as well with "th.". I know that it is not likely to
run into this problem
(such a machine does not exist in real hardware), but it is possible
to trigger this
issue easily and approach 1) would not have this potential issue.

Thanks,
Christoph


>
> Thanks.
>
> ________________________________
> juzhe.zhong@rivai.ai
>
>
> From: Christoph Müllner
> Date: 2023-11-22 18:07
> To: juzhe.zhong@rivai.ai
> CC: gcc-patches; kito.cheng; Kito.cheng; cooper.joshua; Robin Dapp; jeffreyalaw; Philipp Tomsich; Cooper Qu; Jin Ma; Nelson Chu
> Subject: Re: RISC-V: Support XTheadVector extensions
> Hi Juzhe,
>
> Sorry for the late reply, but I was not on CC, so I missed this email.
>
> On Fri, Nov 17, 2023 at 2:41 PM juzhe.zhong@rivai.ai
> <juzhe.zhong@rivai.ai> wrote:
> >
> > Ok. I just read the theadvector extension.
> >
> > https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadvector.adoc
> >
> > Theadvector is not custom extension. Just a uarch to disable some of the RVV1.0 extension
> > Theadvector can be considered as subextension of 'V' extension with disabling some of the
> > instructions and adding some new thead vector target load/store (This is another story).
> >
> > So, for disabling the instruction that theadvector doesn't support.
> > You don't need to touch such many codes.
> >
> > Here is a much simpler approach to do (I think it's definitely working):
> > 1. Don't change any codes in vector.md and keep GCC generates ASM with "th." prefix.
> > 2. Add !TARGET_THEADVECTOR into vector-iterator.md to disable the mode you don't want.
> > For example , theadvector doesn't support fractional vector.
> >
> > Then it's pretty simple:
> >
> > RVVMF2SI "TARGET_VECTOR && !TARGET_THEADVECTOR".
> >
> > 3. Remove all the tests you add in this patch.
> > 4. You can add theadvector specific load/store for example, th.vlb instructions they are allowed.
> > 5. Modify binutils, and make th.vmulh.vv as the pseudo instruction of vmulh.vv
> > 6. So with compile option "-S", you will still see ASM as  "vmulh.vv". but with objdump, you will see th.vmulh.vv.
>
> Yes, all these points sound reasonable, to minimize the patchset size.
> I believe in point 1 you meant "without th. prefix".
>
> I've added Jin Ma (who is the main author of the Binutils patchset) so
> he is also aware
> of the proposal to use pseudo instructions to avoid duplication in Binutils.
>
> Thank you very much!
> Christoph
>
>
> >
> > After this change, you can send V2, then I can continue to review on GCC-15.
> >
> > Thanks.
> >
> > ________________________________
> > juzhe.zhong@rivai.ai
> >
> >
> > From: juzhe.zhong@rivai.ai
> > Date: 2023-11-17 19:39
> > To: gcc-patches
> > CC: kito.cheng; kito.cheng; cooper.joshua; Robin Dapp; jeffreyalaw
> > Subject: RISC-V: Support XTheadVector extensions
> > 90% theadvector extension reusing current RVV 1.0 instructions patterns:
> > Just change ASM, For example:
> >
> > @@ -2923,7 +2923,7 @@ (define_insn "*pred_mulh<v_su><mode>_scalar"
> >       (match_operand:VFULLI_D 3 "register_operand"  "vr,vr, vr, vr")] VMULH)
> >    (match_operand:VFULLI_D 2 "vector_merge_operand" "vu, 0, vu,  0")))]
> >    "TARGET_VECTOR"
> > -  "vmulh<v_su>.vx\t%0,%3,%z4%p1"
> > +  "%^vmulh<v_su>.vx\t%0,%3,%z4%p1"
> >    [(set_attr "type" "vimul")
> >     (set_attr "mode" "<MODE>")])
> >
> > +  if (letter == '^')
> > +    {
> > +      if (TARGET_XTHEADVECTOR)
> > + fputs ("th.", file);
> > +      return;
> > +    }
> >
> >
> > For almost all patterns, you just simply append "th." in the ASM prefix.
> > like change "vmulh.vv" -> "th.vmulh.vv"
> >
> > Almost all theadvector instructions are not new features,  all same as RVV1.0.
> > Why do you invent the such ISA doesn't include any features that RVV1.0 doesn't satisfy ?
> >
> > I am not explicitly object this patch. But I should know the reason.
> >
> > Btw, stage 1 will close soon.  So I will review this patch on GCC-15 as long as all other RISC-V maintainers agree.
> >
> >
> > ________________________________
> > juzhe.zhong@rivai.ai
>