RE: PING**4 - [PATCH] middle-end: Support ABIs that pass FP values as wider integers.

["Roger Sayle" <roger@nextmovesoftware.com>]

[-- Attachment #1: Type: text/plain, Size: 3425 bytes --]


I thought I'd add a few comments that might help reviewers of this patch.
Most importantly, this patch should be completely safe, as both changes
only trigger with FLOAT vs INT size mismatches which currently both ICE in
the compiler a few lines later.  Admittedly, this indicates something odd 
about a target's choice of ABI, but one alternative might be to issue a
"sorry, unimplemented" error message rather than ICE, perhaps with a
TODO or FIXME comment that support for mixed size FP/integer ABIs be
added in future.

The only (other?) possible point of contention is the (arbitrary) decision that
when passing floating point values in a larger integer register, the code is
hardwired to use zero-extension.  This in theory could be turned into a target
hook to support sign-extension, but given these are padding bits, zero seems
appropriate. [On x86_64, if passing DFmode argument in a V2DFmode vector,
say, it seems reasonable to use movq and zero the high bits].

The final point is that at the moment, the only affected target is nvptx-none,
as I don't believe any other backend specifies an ABI that requires passing 
floating point values in wider integer registers.  Having said that, most targets
don't yet support HFmode, and their ABI specifications haven't yet been
updated as what to do in that eventuality.  If they choose to treat these like
HImode, they'll run into the same issues, as most ABIs pass HImode in 
wider word_mode registers.

I hope this helps.  If folks could air their concerns out loud, I can try my best
to address those worries.

Many thanks in advance (and thanks to Tobias and Tom for pushing for this).
Roger
--

> -----Original Message-----
> From: Tom de Vries <tdevries@suse.de>
> Sent: 14 March 2022 08:06
> To: Jeff Law <jeffreyalaw@gmail.com>; Richard Biener <rguenther@suse.de>;
> Tobias Burnus <tobias@codesourcery.com>
> Cc: richard.sandiford@arm.com; Jeff Law <law@redhat.com>; gcc-
> patches@gcc.gnu.org; Roger Sayle <roger@nextmovesoftware.com>
> Subject: PING**4 - [PATCH] middle-end: Support ABIs that pass FP values as
> wider integers.
> 
> On 3/2/22 20:18, Jeff Law via Gcc-patches wrote:
> >
> >
> > On 2/28/2022 5:54 AM, Richard Biener via Gcc-patches wrote:
> >> On Mon, 28 Feb 2022, Tobias Burnus wrote:
> >>
> >>> Ping**3
> >>>
> >>> On 23.02.22 09:42, Tobias Burnus wrote:
> >>>> PING**2 for the ME review or at least comments to that patch, which
> >>>> fixes a build issue/ICE with nvptx
> >>>>
> >>>> Patch:
> >>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590139.html
> >>>> (for gcc/cfgexpand.cc + gcc/expr.cc)
> >>>>
> >>>> (There is some discussion by Tom and Roger about the BE in the
> >>>> patch thread, which only not relate to the ME patch. But there is
> >>>> no ME-patch comment so far.)
> >>> The related BE patch has been already committed, but to be
> >>> effective, it needs the ME patch.
> >> I'm not sure I'm qualified to review this - maybe Richard is.
> > I'd initially ignored the patch as it didn't seem a good fit for
> > stage4, subsequent messages changed my mind about it, but I never went
> > back to take a deeper look at Roger's patch.
> 
> Ping.
> 
> [ FWIW, I'd appreciate it if a response came before the end of stage 4, such that
> I have some time left to deal with fallout in case the patch is not approved. ]
> 
> Thanks,
> - Tom

[-- Attachment #2: patchi4.txt --]
[-- Type: text/plain, Size: 2206 bytes --]

diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
index d51af2e..c377f16 100644
--- a/gcc/cfgexpand.cc
+++ b/gcc/cfgexpand.cc
@@ -3715,7 +3715,22 @@ expand_value_return (rtx val)
         mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1);
 
       if (mode != old_mode)
-	val = convert_modes (mode, old_mode, val, unsignedp);
+	{
+	  /* Some ABIs require scalar floating point modes to be returned
+	     in a wider scalar integer mode.  We need to explicitly
+	     reinterpret to an integer mode of the correct precision
+	     before extending to the desired result.  */
+	  if (SCALAR_INT_MODE_P (mode)
+	      && SCALAR_FLOAT_MODE_P (old_mode)
+	      && known_gt (GET_MODE_SIZE (mode), GET_MODE_SIZE (old_mode)))
+	    {
+	      scalar_int_mode imode = int_mode_for_mode (old_mode).require ();
+	      val = force_reg (imode, gen_lowpart (imode, val));
+	      val = convert_modes (mode, imode, val, 1);
+	    }
+	  else
+	    val = convert_modes (mode, old_mode, val, unsignedp);
+	}
 
       if (GET_CODE (return_reg) == PARALLEL)
 	emit_group_load (return_reg, val, type, int_size_in_bytes (type));
diff --git a/gcc/expr.cc b/gcc/expr.cc
index 35e4029..e4efdcd 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -10674,6 +10674,19 @@ expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
 	    pmode = promote_ssa_mode (ssa_name, &unsignedp);
 	  gcc_assert (GET_MODE (decl_rtl) == pmode);
 
+	  /* Some ABIs require scalar floating point modes to be passed
+	     in a wider scalar integer mode.  We need to explicitly
+	     truncate to an integer mode of the correct precision before
+	     using a SUBREG to reinterpret as a floating point value.  */
+	  if (SCALAR_FLOAT_MODE_P (mode)
+	      && SCALAR_INT_MODE_P (pmode)
+	      && known_lt (GET_MODE_SIZE (mode), GET_MODE_SIZE (pmode)))
+	    {
+	      scalar_int_mode imode = int_mode_for_mode (mode).require ();
+	      temp = force_reg (imode, gen_lowpart (imode, decl_rtl));
+	      return gen_lowpart_SUBREG (mode, temp);
+	    }
+
 	  temp = gen_lowpart_SUBREG (mode, decl_rtl);
 	  SUBREG_PROMOTED_VAR_P (temp) = 1;
 	  SUBREG_PROMOTED_SET (temp, unsignedp);