Re: [PATCH] expmed: TRUNCATE value1 if needed in store_bit_field_using_insv

[Jeff Law]

On 6/5/24 8:57 AM, YunQiang Su wrote:
Richard Sandiford <richard.sandif...@arm.com> 于2024年6月5日周三 22:14写道：

YunQiang Su <s...@gcc.gnu.org> writes:
PR target/113179.

In `store_bit_field_using_insv`, we just use SUBREG if value_mode
= op_mode, while in some ports, a sign_extend will be needed,
such as MIPS64:
   If either GPR rs or GPR rt does not contain sign-extended 32-bit
   values (bits 63..31 equal), then the result of the operation is
   UNPREDICTABLE.

The problem happens for the code like:
   struct xx {
         int a:4;
         int b:24;
         int c:3;
         int d:1;
   };

   void xx (struct xx *a, long long b) {
         a->d = b;
   }

In the above code, the hard register contains `b`, may be note well
sign-extended.

gcc/
       PR target/113179
       * expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
       needed.

gcc/testsuite
       PR target/113179
       * gcc.target/mips/pr113179.c: New tests.
---
  gcc/expmed.cc                            | 12 +++++++++---
  gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
  2 files changed, 27 insertions(+), 3 deletions(-)
  create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c

diff --git a/gcc/expmed.cc b/gcc/expmed.cc
index 4ec035e4843..6a582593da8 100644
--- a/gcc/expmed.cc
+++ b/gcc/expmed.cc
@@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn *insv, 
rtx op0,
           }
         else
           {
-           tmp = gen_lowpart_if_possible (op_mode, value1);
-           if (! tmp)
-             tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
+           if (targetm.mode_rep_extended (op_mode, value_mode))
+             tmp = simplify_gen_unary (TRUNCATE, op_mode,
+                                       value1, value_mode);
+           else
+             {
+               tmp = gen_lowpart_if_possible (op_mode, value1);
+               if (! tmp)
+                 tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
+             }
           }
         value1 = tmp;
       }

I notice this patch is already applied.  Was it approved?  I didn't
see an approval in my feed or in the archives.


Sorry. I was supposed that it only effects MIPS targets since only MIPS defines
   targetm.mode_rep_extended
Just a note for the future, even if something is guarded by a target 
hook that's only defined by a single target we'd want to see an ACK if 
the code is in a target independent file.

Jeff