Re: [PATCH, rs6000] Remove mode promotion of SSA variables

[HAO CHEN GUI via Gcc-patches]

On 19/5/2021 下午 9:20, Segher Boessenkool wrote:
Hi!

On Wed, May 19, 2021 at 04:36:00PM +0800, HAO CHEN GUI wrote:
On 19/5/2021 下午 4:33, HAO CHEN GUI wrote:
     This patch removes mode promotion of SSA variables on rs6000
platform.
It isn't "promotion of SSA variables".  At the point where this code
applies we are generating RTL, which doesn't do SSA.  It has what is
called "pseudo-registers" (or short, "pseudos"), which will be assigned
hard registers later.

     Bootstrapped and tested on powerppc64le and powerppc64be (with
m32) with no regressions. Is this okay for trunk? Any recommendations?
Thanks a lot.
powerpc64-linux and powerpc64le-linux I guess?

        * config/rs6000/rs6000-call.c (rs6000_promote_function_mode):
        Replace PROMOTE_MODE marco with its content.
        * config/rs6000/rs6000.h (PROMOTE_MODE): Remove.
Please split this into two?  The first is obvious, the second much less
so; we'll need to see justification for it.  I know it helps greatly,
but please record that in the commit message :-)

diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
index f5676255387..dca139b2ecf 100644
--- a/gcc/config/rs6000/rs6000-call.c
+++ b/gcc/config/rs6000/rs6000-call.c
@@ -6646,7 +6646,9 @@ rs6000_promote_function_mode (const_tree type 
ATTRIBUTE_UNUSED,
                              int *punsignedp ATTRIBUTE_UNUSED,
                              const_tree, int for_return ATTRIBUTE_UNUSED)
  {
-  PROMOTE_MODE (mode, *punsignedp, type);
+  if (GET_MODE_CLASS (mode) == MODE_INT
+      && GET_MODE_SIZE (mode) < (TARGET_32BIT ? 4 : 8))
+    mode = TARGET_32BIT ? SImode : DImode;
  
    return mode;
  }
So this part is pre-approved as a separate patch.

-/* Define this macro if it is advisable to hold scalars in registers
-   in a wider mode than that declared by the program.  In such cases,
-   the value is constrained to be within the bounds of the declared
-   type, but kept valid in the wider mode.  The signedness of the
-   extension may differ from that of the type.  */
-
-#define PROMOTE_MODE(MODE,UNSIGNEDP,TYPE)      \
-  if (GET_MODE_CLASS (MODE) == MODE_INT                \
-      && GET_MODE_SIZE (MODE) < (TARGET_32BIT ? 4 : 8)) \
-    (MODE) = TARGET_32BIT ? SImode : DImode;
-
And this part needs some more words in the commit message :-)

Since we do have instructions that can do (almost) everything 32-bit at
least as efficiently as the corresponding 64-bit things, it helps a lot
to not promote so many things to 64 bits.  We didn't realise that before
because that TARGET_PROMOTE_FUNCTION_MODE thing was in the way, since
the very early days of the rs6000 port even, so everyone (well, at least
me :-) ) was tricked into thinking this is an ABI requirement and we
cannot touch it.  But of course it is not, and we can :-)

Some examples of how this improves generated code, or even some
benchmark results, would be good to have.

Also, how about something like

#define PROMOTE_MODE(MODE,UNSIGNEDP,TYPE)       \
   if (GET_MODE_CLASS (MODE) == MODE_INT                \
       && GET_MODE_SIZE (MODE) < 4)          \
     (MODE) = SImode;

(that is, promoting modes smaller than SImode to SImode).  How does that
compare?

It hits an ICE when assigning a function return value to a variable. The 
mode of variable is promoted to SImode, but the mode of return value is 
promoted to DImode. Current GCC logical is either the mode of set_dest 
is unchanged or the mode of set_dest matches the mode of function return 
value.

By the way, I tested the patch on ppc64 with m32 option. The SPECint 
shows a little improvement(0.4%). So it's better not do any mode promotions.


Thanks!


Segher