diff mbox series

RISC-V: Adjusting the comments of the emit_vlmax_insn/emit_vlmax_insn_lra/emit_nonvlmax_insn functions

Message ID 20230921071118.3321383-1-lehua.ding@rivai.ai
State Unresolved
Headers
Series RISC-V: Adjusting the comments of the emit_vlmax_insn/emit_vlmax_insn_lra/emit_nonvlmax_insn functions |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Lehua Ding Sept. 21, 2023, 7:11 a.m. UTC 
  This patch adjusts the comments of the
emit_vlmax_insn/emit_vlmax_insn_lra/emit_nonvlmax_insn functions.
The purpose of the adjustment is to make it clear that vlmax here is not
VLMAX as defined inside the RVV ISA. This is because this function is used
by RVV mode (e.g. RVVM1SImode) in addition to VLS mode (V16QI). For RVV mode,
it means the same thing, for VLS mode, it indicates setting the vl to the
number of units of the mode. Changed the comment because I didn't think of
a better name. If there is a suitable name, feel free to discuss it.

gcc/ChangeLog:

	* config/riscv/riscv-v.cc (emit_nonvlmax_insn): Adjust comments.
	(emit_vlmax_insn_lra): Adjust comments.

---
 gcc/config/riscv/riscv-v.cc | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

--
2.36.3

Comments

Robin Dapp Sept. 21, 2023, 9:07 a.m. UTC | #1 
Hi Lehua,

I once had different comments for those but either I never pushed them
or they got buried in the process of refactoring.  The explanatory
comment explaining vlmax is also in "nowhere land" below autovec_use_vlmax_p.
(it says vsetvli instead of vsetvl as well...)  It would be useful
to move it to above the function comments you touch.

> +/* Emit RVV insn which vl is the number of units of the vector mode.
> +   This function can only be used before LRA pass or for VLS_AVL_IMM modes.  */

Emit an RVV insn with a vector length that equals the number of units of
the vector mode.  For VLA modes this corresponds to VLMAX.

Unless the vector length can be encoded in the vsetivl[i] instruction this
function must only be used as long as we can create pseudo registers.
This is because it will set a pseudo register to VLMAX using vsetvl and
use this as definition for the vector length.


Besides, we could add a const_vlmax_p () || can_create_pseudo_p assert here?


> +/* Like emit_vlmax_insn but can be only used after LRA pass that can't create
> +   pseudo register.  */

Like emit_vlmax_insn but must only be used when we cannot create pseudo
registers anymore.  This function, however, takes a predefined vector
length from the value in VL.

> +/* Emit RVV insn which vl is the VL argument.  */
> +emit_nonvlmax_insn (unsigned icode, unsigned insn_flags, rtx *ops, rtx vl)

I think I renamed this to emit_len_insn or something before but Juzhe didn't
like it ;)

How about something like:
Emit an RVV insn with a predefined vector length.  Contrary to emit_vlmax_insn
the instruction's vector length is not deduced from its mode but taken from 
the value in VL.

Regards
 Robin
Lehua Ding Sept. 21, 2023, 9:51 a.m. UTC | #2 
Hi Robin,

> I once had different comments for those but either I never pushed them
> or they got buried in the process of refactoring.  The explanatory
> comment explaining vlmax is also in "nowhere land" below autovec_use_vlmax_p.
> (it says vsetvli instead of vsetvl as well...)  It would be useful
> to move it to above the function comments you touch.

I would like to move this comment to insn_expander::emit_insn body 
before set avl in another patch which add VLS avl_type.

> 
>> +/* Emit RVV insn which vl is the number of units of the vector mode.
>> +   This function can only be used before LRA pass or for VLS_AVL_IMM modes.  */
> 
> Emit an RVV insn with a vector length that equals the number of units of
> the vector mode.  For VLA modes this corresponds to VLMAX.
> 
> Unless the vector length can be encoded in the vsetivl[i] instruction this
> function must only be used as long as we can create pseudo registers.
> This is because it will set a pseudo register to VLMAX using vsetvl and
> use this as definition for the vector length.
> 
> 
> Besides, we could add a const_vlmax_p () || can_create_pseudo_p assert here?
> 
> 
>> +/* Like emit_vlmax_insn but can be only used after LRA pass that can't create
>> +   pseudo register.  */
> 
> Like emit_vlmax_insn but must only be used when we cannot create pseudo
> registers anymore.  This function, however, takes a predefined vector
> length from the value in VL.
> 
>> +/* Emit RVV insn which vl is the VL argument.  */
>> +emit_nonvlmax_insn (unsigned icode, unsigned insn_flags, rtx *ops, rtx vl)
> 
> I think I renamed this to emit_len_insn or something before but Juzhe didn't
> like it ;)
> 
> How about something like:
> Emit an RVV insn with a predefined vector length.  Contrary to emit_vlmax_insn
> the instruction's vector length is not deduced from its mode but taken from
> the value in VL.

Thank you very much, I used all of them. Here the V2 patch:
https://gcc.gnu.org/pipermail/gcc-patches/2023-September/631114.html
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
index 64a71a128d4..df4d2ac1b2b 100644
--- a/gcc/config/riscv/riscv-v.cc
+++ b/gcc/config/riscv/riscv-v.cc
@@ -347,9 +347,8 @@  private:
   expand_operand m_ops[MAX_OPERANDS];
 };

-/* Emit RVV insn which vl is VLMAX.
-   This function can only be used before LRA pass or
-   for VLS_AVL_IMM modes.  */
+/* Emit RVV insn which vl is the number of units of the vector mode.
+   This function can only be used before LRA pass or for VLS_AVL_IMM modes.  */
 void
 emit_vlmax_insn (unsigned icode, unsigned insn_flags, rtx *ops)
 {
@@ -357,23 +356,23 @@  emit_vlmax_insn (unsigned icode, unsigned insn_flags, rtx *ops)
   e.emit_insn ((enum insn_code) icode, ops);
 }

-/* Emit RVV insn which vl is VL.  */
+/* Like emit_vlmax_insn but can be only used after LRA pass that can't create
+   pseudo register.  */
 void
-emit_nonvlmax_insn (unsigned icode, unsigned insn_flags, rtx *ops, rtx vl)
+emit_vlmax_insn_lra (unsigned icode, unsigned insn_flags, rtx *ops, rtx vl)
 {
-  insn_expander<RVV_INSN_OPERANDS_MAX> e (insn_flags, false);
+  gcc_assert (!can_create_pseudo_p ());
+
+  insn_expander<RVV_INSN_OPERANDS_MAX> e (insn_flags, true);
   e.set_vl (vl);
   e.emit_insn ((enum insn_code) icode, ops);
 }

-/* Emit RVV insn which vl is VL but the AVL_TYPE insn attr is VLMAX.
-   This function used after LRA pass that cann't create pseudo register.  */
+/* Emit RVV insn which vl is the VL argument.  */
 void
-emit_vlmax_insn_lra (unsigned icode, unsigned insn_flags, rtx *ops, rtx vl)
+emit_nonvlmax_insn (unsigned icode, unsigned insn_flags, rtx *ops, rtx vl)
 {
-  gcc_assert (!can_create_pseudo_p ());
-
-  insn_expander<RVV_INSN_OPERANDS_MAX> e (insn_flags, true);
+  insn_expander<RVV_INSN_OPERANDS_MAX> e (insn_flags, false);
   e.set_vl (vl);
   e.emit_insn ((enum insn_code) icode, ops);
 }