diff mbox series

RISC-V: Fix register class subset checks for CLASS_MAX_NREGS

Message ID alpine.DEB.2.20.2111011643350.18331@tpp.orcam.me.uk
State Committed
Commit a31056e9196daf0a5b0e92d171b5227cc994103b
Headers
Series RISC-V: Fix register class subset checks for CLASS_MAX_NREGS |

Commit Message

Maciej W. Rozycki Nov. 2, 2021, 3:17 p.m. UTC 
  Fix the register class subset checks in the determination of the maximum 
number of consecutive registers needed to hold a value of a given mode.  

The number depends on whether a register is a general-purpose or a 
floating-point register, so check whether the register class requested 
is a subset (argument 1 to `reg_class_subset_p') rather than superset 
(argument 2) of GR_REGS or FP_REGS class respectively.

	gcc/
	* config/riscv/riscv.c (riscv_class_max_nregs): Swap the 
	arguments to `reg_class_subset_p'.
---
Hi,

 This looks like a thinko to me, but please double-check I have not become 
confused myself.

 My understanding is that current code works, because the SIBCALL_REGS and 
JALR_REGS classes are only ever used for addresses, which won't ever span 
multiple registers, and then the only class other than ALL_REGS that 
inludes floating-point registers is FP_REGS.  Therefore the hook will only 
ever see either GR_REGS or FP_REGS as the class enquired about and 
consequently the result of the `reg_class_subset_p' calls will be the same 
regardless of the order of the arguments chosen.  We should be getting the 
semantics right regardless.

 Regression-tested with the `riscv64-linux-gnu' target.  OK to apply?

  Maciej
---
 gcc/config/riscv/riscv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

gcc-riscv-class-max-nregs.diff

Comments

Kito Cheng Nov. 2, 2021, 5 p.m. UTC | #1 
Hi Maciej:

LGTM, My first impression of this patch is also confusing about the
ordering of arguments for reg_class_subset_p, but after I double
checked that with gdb and read the comment above the
reg_class_subset_p, I think this change is right.

Thanks!


On Tue, Nov 2, 2021 at 11:17 PM Maciej W. Rozycki <macro@embecosm.com> wrote:
>
> Fix the register class subset checks in the determination of the maximum
> number of consecutive registers needed to hold a value of a given mode.
>
> The number depends on whether a register is a general-purpose or a
> floating-point register, so check whether the register class requested
> is a subset (argument 1 to `reg_class_subset_p') rather than superset
> (argument 2) of GR_REGS or FP_REGS class respectively.
>
>         gcc/
>         * config/riscv/riscv.c (riscv_class_max_nregs): Swap the
>         arguments to `reg_class_subset_p'.
> ---
> Hi,
>
>  This looks like a thinko to me, but please double-check I have not become
> confused myself.
>
>  My understanding is that current code works, because the SIBCALL_REGS and
> JALR_REGS classes are only ever used for addresses, which won't ever span
> multiple registers, and then the only class other than ALL_REGS that
> inludes floating-point registers is FP_REGS.  Therefore the hook will only
> ever see either GR_REGS or FP_REGS as the class enquired about and
> consequently the result of the `reg_class_subset_p' calls will be the same
> regardless of the order of the arguments chosen.  We should be getting the
> semantics right regardless.
>
>  Regression-tested with the `riscv64-linux-gnu' target.  OK to apply?
>
>   Maciej
> ---
>  gcc/config/riscv/riscv.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> gcc-riscv-class-max-nregs.diff
> Index: gcc/gcc/config/riscv/riscv.c
> ===================================================================
> --- gcc.orig/gcc/config/riscv/riscv.c
> +++ gcc/gcc/config/riscv/riscv.c
> @@ -4811,10 +4811,10 @@ riscv_modes_tieable_p (machine_mode mode
>  static unsigned char
>  riscv_class_max_nregs (reg_class_t rclass, machine_mode mode)
>  {
> -  if (reg_class_subset_p (FP_REGS, rclass))
> +  if (reg_class_subset_p (rclass, FP_REGS))
>      return riscv_hard_regno_nregs (FP_REG_FIRST, mode);
>
> -  if (reg_class_subset_p (GR_REGS, rclass))
> +  if (reg_class_subset_p (rclass, GR_REGS))
>      return riscv_hard_regno_nregs (GP_REG_FIRST, mode);
>
>    return 0;
Maciej W. Rozycki Nov. 3, 2021, 5:07 p.m. UTC | #2 
On Wed, 3 Nov 2021, Kito Cheng wrote:

> LGTM, My first impression of this patch is also confusing about the
> ordering of arguments for reg_class_subset_p, but after I double
> checked that with gdb and read the comment above the
> reg_class_subset_p, I think this change is right.

 Applied now, thanks for your review.

  Maciej
diff mbox series

Patch

Index: gcc/gcc/config/riscv/riscv.c
===================================================================
--- gcc.orig/gcc/config/riscv/riscv.c
+++ gcc/gcc/config/riscv/riscv.c
@@ -4811,10 +4811,10 @@  riscv_modes_tieable_p (machine_mode mode
 static unsigned char
 riscv_class_max_nregs (reg_class_t rclass, machine_mode mode)
 {
-  if (reg_class_subset_p (FP_REGS, rclass))
+  if (reg_class_subset_p (rclass, FP_REGS))
     return riscv_hard_regno_nregs (FP_REG_FIRST, mode);
 
-  if (reg_class_subset_p (GR_REGS, rclass))
+  if (reg_class_subset_p (rclass, GR_REGS))
     return riscv_hard_regno_nregs (GP_REG_FIRST, mode);
 
   return 0;