[PATCH 0/3] Miscll fixlets

[PATCH 0/3] Miscll fixlets

[Vineet Gupta]

Hi,

Spring cleaning of a really old branch.
stage1 might be the right time to get them in.

Thx,
-Vineet

Vineet Gupta (3):
  docs: rtl: document GET_MODE_INNER
  RISC-V: miscll comment fixes [NFC]
  combine: initialize a local var

 gcc/combine.cc            | 2 +-
 gcc/config/riscv/riscv.cc | 6 ++++--
 gcc/config/riscv/riscv.h  | 2 +-
 gcc/doc/rtl.texi          | 4 ++++
 4 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH 1/3] docs: rtl: document GET_MODE_INNER

[Vineet Gupta]

gcc/ChangeLog
	* doc/rtl.texi: Add entry for GET_MODE_INNER.

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 gcc/doc/rtl.texi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
index 8ea6588cb71f..f1643f41dfc6 100644
--- a/gcc/doc/rtl.texi
+++ b/gcc/doc/rtl.texi
@@ -1582,6 +1582,10 @@ less than or equal to @code{HOST_BITS_PER_INT}.
 @item GET_MODE_ALIGNMENT (@var{m})
 Return the required alignment, in bits, for an object of mode @var{m}.
 
+@findex GET_MODE_INNER
+@item GET_MODE_INNER (@var{m})
+Returns the mode of subunits contained in a complex mode such as Vector.
+
 @findex GET_MODE_UNIT_SIZE
 @item GET_MODE_UNIT_SIZE (@var{m})
 Returns the size in bytes of the subunits of a datum of mode @var{m}.
-- 
2.34.1

Re: [PATCH 1/3] docs: rtl: document GET_MODE_INNER

[Jeff Law]

On 5/2/24 12:59 PM, Vineet Gupta wrote:
> gcc/ChangeLog
> 	* doc/rtl.texi: Add entry for GET_MODE_INNER.
> 
> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> ---
>   gcc/doc/rtl.texi | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
> index 8ea6588cb71f..f1643f41dfc6 100644
> --- a/gcc/doc/rtl.texi
> +++ b/gcc/doc/rtl.texi
> @@ -1582,6 +1582,10 @@ less than or equal to @code{HOST_BITS_PER_INT}.
>   @item GET_MODE_ALIGNMENT (@var{m})
>   Return the required alignment, in bits, for an object of mode @var{m}.
>   
> +@findex GET_MODE_INNER
> +@item GET_MODE_INNER (@var{m})
> +Returns the mode of subunits contained in a complex mode such as Vector.
I'd probably use the macro's docs nearly verbatim:

> /* Return the mode of the basic parts of MODE.  For vector modes this is the
>    mode of the vector elements.  For complex modes it is the mode of the real
>    and imaginary parts.  For other modes it is MODE itself.  */



OK with that change.

jeff

[PATCH v2 1/3] docs: rtl: document GET_MODE_INNER

[Vineet Gupta]

gcc/ChangeLog
	* doc/rtl.texi: Add entry for GET_MODE_INNER.

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 gcc/doc/rtl.texi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
index 8ea6588cb71f..aa10b5235b53 100644
--- a/gcc/doc/rtl.texi
+++ b/gcc/doc/rtl.texi
@@ -1582,6 +1582,13 @@ less than or equal to @code{HOST_BITS_PER_INT}.
 @item GET_MODE_ALIGNMENT (@var{m})
 Return the required alignment, in bits, for an object of mode @var{m}.
 
+@findex GET_MODE_INNER
+@item GET_MODE_INNER (@var{m})
+Returns the mode of the basic parts of mode @var{m}.  For vector modes
+this is the mode of the vector elements.  For complex modes it is the
+mode of the real and imaginary parts.  For other modes it is mode @var{m}
+itself.
+
 @findex GET_MODE_UNIT_SIZE
 @item GET_MODE_UNIT_SIZE (@var{m})
 Returns the size in bytes of the subunits of a datum of mode @var{m}.
-- 
2.34.1

[Committed 1/2] docs: rtl: document GET_MODE_INNER

[Vineet Gupta]

gcc/ChangeLog
	* doc/rtl.texi: Add entry for GET_MODE_INNER.

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 gcc/doc/rtl.texi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
index 8ea6588cb71f..aa10b5235b53 100644
--- a/gcc/doc/rtl.texi
+++ b/gcc/doc/rtl.texi
@@ -1582,6 +1582,13 @@ less than or equal to @code{HOST_BITS_PER_INT}.
 @item GET_MODE_ALIGNMENT (@var{m})
 Return the required alignment, in bits, for an object of mode @var{m}.
 
+@findex GET_MODE_INNER
+@item GET_MODE_INNER (@var{m})
+Returns the mode of the basic parts of mode @var{m}.  For vector modes
+this is the mode of the vector elements.  For complex modes it is the
+mode of the real and imaginary parts.  For other modes it is mode @var{m}
+itself.
+
 @findex GET_MODE_UNIT_SIZE
 @item GET_MODE_UNIT_SIZE (@var{m})
 Returns the size in bytes of the subunits of a datum of mode @var{m}.
-- 
2.34.1

[PATCH 2/3] RISC-V: miscll comment fixes [NFC]

[Vineet Gupta]

gcc/ChangeLog:
	* config/riscv/riscv.cc: Comment updates.
	* config/riscv/riscv.h: Ditto.

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 gcc/config/riscv/riscv.cc | 6 ++++--
 gcc/config/riscv/riscv.h  | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 24d1ead3902c..79994a5ddf32 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -1219,7 +1219,9 @@ riscv_legitimate_constant_p (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
   return riscv_const_insns (x) > 0;
 }
 
-/* Implement TARGET_CANNOT_FORCE_CONST_MEM.  */
+/* Implement TARGET_CANNOT_FORCE_CONST_MEM.
+   Return true if X cannot (or should not) be spilled to the
+   constant pool.  */
 
 static bool
 riscv_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
@@ -8585,7 +8587,7 @@ riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
 	       && GET_MODE_CLASS (mode2) == MODE_FLOAT));
 }
 
-/* Implement CLASS_MAX_NREGS.  */
+/* Implement TARGET_CLASS_MAX_NREGS.  */
 
 static unsigned char
 riscv_class_max_nregs (reg_class_t rclass, machine_mode mode)
diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
index 7797e67317a6..58d0b09bf7d9 100644
--- a/gcc/config/riscv/riscv.h
+++ b/gcc/config/riscv/riscv.h
@@ -315,7 +315,7 @@ ASM_MISA_SPEC
 	- FRAME_POINTER_REGNUM
    - 1 vl register
    - 1 vtype register
-   - 30 unused registers for future expansion
+   - 28 unused registers for future expansion
    - 32 vector registers  */
 
 #define FIRST_PSEUDO_REGISTER 128
-- 
2.34.1

Re: [PATCH 2/3] RISC-V: miscll comment fixes [NFC]

[Jeff Law]

On 5/2/24 12:59 PM, Vineet Gupta wrote:
> gcc/ChangeLog:
> 	* config/riscv/riscv.cc: Comment updates.
> 	* config/riscv/riscv.h: Ditto.
OK
jeff

[Committed 2/2] RISC-V: miscll comment fixes [NFC]

[Vineet Gupta]

gcc/ChangeLog:
	* config/riscv/riscv.cc: Comment updates.
	* config/riscv/riscv.h: Ditto.

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 gcc/config/riscv/riscv.cc | 6 ++++--
 gcc/config/riscv/riscv.h  | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 8ed9df8126a6..44945d47fd64 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -1258,7 +1258,9 @@ riscv_legitimate_constant_p (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
   return riscv_const_insns (x) > 0;
 }
 
-/* Implement TARGET_CANNOT_FORCE_CONST_MEM.  */
+/* Implement TARGET_CANNOT_FORCE_CONST_MEM.
+   Return true if X cannot (or should not) be spilled to the
+   constant pool.  */
 
 static bool
 riscv_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
@@ -8624,7 +8626,7 @@ riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
 	       && GET_MODE_CLASS (mode2) == MODE_FLOAT));
 }
 
-/* Implement CLASS_MAX_NREGS.  */
+/* Implement TARGET_CLASS_MAX_NREGS.  */
 
 static unsigned char
 riscv_class_max_nregs (reg_class_t rclass, machine_mode mode)
diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
index 7797e67317a6..58d0b09bf7d9 100644
--- a/gcc/config/riscv/riscv.h
+++ b/gcc/config/riscv/riscv.h
@@ -315,7 +315,7 @@ ASM_MISA_SPEC
 	- FRAME_POINTER_REGNUM
    - 1 vl register
    - 1 vtype register
-   - 30 unused registers for future expansion
+   - 28 unused registers for future expansion
    - 32 vector registers  */
 
 #define FIRST_PSEUDO_REGISTER 128
-- 
2.34.1

[PATCH 3/3] combine: initialize a local var

[Vineet Gupta]

This is no logic change (but technically still a functional change).

Ran into this when stepping thru combine code.
@newpat has some random garbage for a bit until it is actually set.
With the fix it remains 0 until actually set.

gcc/ChangeLog:
	* combine.cc (try_combine): Initialize newpat.

CC: Segher Boessenkool <segher@kernel.crashing.org>
Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 gcc/combine.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/combine.cc b/gcc/combine.cc
index 92b8d98e6c15..0b5fe00c8c5b 100644
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -2522,7 +2522,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
 	     bool *new_direct_jump_p, rtx_insn *last_combined_insn)
 {
   /* New patterns for I3 and I2, respectively.  */
-  rtx newpat, newi2pat = 0;
+  rtx newpat = 0, newi2pat = 0;
   rtvec newpat_vec_with_clobbers = 0;
   bool substed_i2 = false, substed_i1 = false, substed_i0 = false;
   /* Indicates need to preserve SET in I0, I1 or I2 in I3 if it is not
-- 
2.34.1

Re: [PATCH 3/3] combine: initialize a local var

[Jeff Law]

On 5/2/24 12:59 PM, Vineet Gupta wrote:
> This is no logic change (but technically still a functional change).
> 
> Ran into this when stepping thru combine code.
> @newpat has some random garbage for a bit until it is actually set.
> With the fix it remains 0 until actually set.
> 
> gcc/ChangeLog:
> 	* combine.cc (try_combine): Initialize newpat.
Isn't the same true even after this change if you turn on the optimizer? 
  And isn't the same true for many other objects that are initialized 
lazily?

Ultimately I'll defer to Segher on this.

jeff

Re: [PATCH 3/3] combine: initialize a local var

[Vineet Gupta]

On 5/2/24 13:38, Jeff Law wrote:
>
> On 5/2/24 12:59 PM, Vineet Gupta wrote:
>> This is no logic change (but technically still a functional change).
>>
>> Ran into this when stepping thru combine code.
>> @newpat has some random garbage for a bit until it is actually set.
>> With the fix it remains 0 until actually set.
>>
>> gcc/ChangeLog:
>> 	* combine.cc (try_combine): Initialize newpat.
> Isn't the same true even after this change if you turn on the optimizer? 

You are right. And the reason I was seeing a difference at all is all my
local toolchain builds are -g3 -O0 :-)

> And isn't the same true for many other objects that are initialized 
> lazily?
>
> Ultimately I'll defer to Segher on this.

Of course.

Thx for taking a look.
-Vineet

Re: [PATCH 3/3] combine: initialize a local var

[Segher Boessenkool]

On Thu, May 02, 2024 at 02:38:12PM -0600, Jeff Law wrote:
> 
> 
> On 5/2/24 12:59 PM, Vineet Gupta wrote:
> >This is no logic change (but technically still a functional change).
> >
> >Ran into this when stepping thru combine code.
> >@newpat has some random garbage for a bit until it is actually set.
> >With the fix it remains 0 until actually set.
> >
> >gcc/ChangeLog:
> >	* combine.cc (try_combine): Initialize newpat.
> Isn't the same true even after this change if you turn on the optimizer? 
>  And isn't the same true for many other objects that are initialized 
> lazily?

For all, even.

Without this change the compiler can (in theory, anyway) diagnose the
uninitialised use.  After, there *is* no uninitialised use anymore.

Please don't do this.  It is not an improvement, it is several steps
back, to satisfy a misguides sense of "security".


Segher

Re: [PATCH 3/3] combine: initialize a local var

[Segher Boessenkool]

On Thu, May 02, 2024 at 11:59:24AM -0700, Vineet Gupta wrote:
> This is no logic change (but technically still a functional change).

Where are 1/3 and 2/3?  Or are those unrelated?  Please don't make
series like that.

> Ran into this when stepping thru combine code.
> @newpat has some random garbage for a bit until it is actually set.
> With the fix it remains 0 until actually set.

The same is true for all uninitialised variables.  Setting everything
to zero explicitly is a) quite a bit slower, and b) just as wrong!
For example, here, newpat should never be zero.  Never.  It does not
make any sense.

Is there any place where newpat is used uninitialised?


Segher

Re: [PATCH 3/3] combine: initialize a local var

[Vineet Gupta]

On 5/3/24 01:26, Segher Boessenkool wrote:
> On Thu, May 02, 2024 at 11:59:24AM -0700, Vineet Gupta wrote:
>> This is no logic change (but technically still a functional change).
> Where are 1/3 and 2/3?  Or are those unrelated? 

Yes they were unrelated (minor doc fixes) hence didn't want to spam you,
but you are right just because they were piled up in my spring cleaning
branch doesn't mean I send them out that way.

> Please don't make series like that.

Noted for future.

>
>> Ran into this when stepping thru combine code.
>> @newpat has some random garbage for a bit until it is actually set.
>> With the fix it remains 0 until actually set.
> The same is true for all uninitialised variables.  Setting everything
> to zero explicitly is a) quite a bit slower, and b) just as wrong!
> For example, here, newpat should never be zero.  Never.  It does not
> make any sense.
>
> Is there any place where newpat is used uninitialised?

As I mentioned this patch was the outcome of my "debugging" experience
and agree I should not conflate between what the compiler does and what
is needed to debug the compiler itself.

I've dropped this one.

Thx,
-Vineet