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

[Richard Biener <rguenther@suse.de>]

On Mon, 14 Mar 2022, Roger Sayle wrote:

> 
> Hi Richard,
> 
> > The above seems backwards - it should be (subreg:HF (reg:SI 0)), no?
> 
> Line 949 of emit-rtl.cc in the definition of "validate_subreg" contains:
> 
>   /* Subregs involving floating point modes are not allowed to
>      change size.  Therefore (subreg:DI (reg:DF) 0) is fine, but
>      (subreg:SI (reg:DF) 0) isn't.  */
>   else if (FLOAT_MODE_P (imode) || FLOAT_MODE_P (omode))
>     {
>       if (! (known_eq (isize, osize)
> 
> Hence, we ICE on line 1022 of emit-rtl.cc in gen_rtx_SUBREG, that asserts
> validate_subreg, when called with omode=HFmode and imode=SImode.

Yes, which is why I think the target should claim argument passing
happens in reg:HI.  But as said, I'm not very familiar with argument
passing internals.  But still the patch looks quite bolted on.

For the expand_value_return I have to guess that 'val' has HFmode,
the DECL_RESULT also has HFmode(?), promote_function_mode then
returns SImode which isn't supported.  So make it return HImode?

> Cheers,
> Roger
> --
> 
> > -----Original Message-----
> > From: Richard Biener <rguenther@suse.de>
> > Sent: 14 March 2022 10:15
> > To: Roger Sayle <roger@nextmovesoftware.com>
> > Cc: 'Tom de Vries' <tdevries@suse.de>; 'Jeff Law' <jeffreyalaw@gmail.com>;
> > 'Tobias Burnus' <tobias@codesourcery.com>; richard.sandiford@arm.com;
> 'Jeff
> > Law' <law@redhat.com>; gcc-patches@gcc.gnu.org
> > Subject: RE: PING**4 - [PATCH] middle-end: Support ABIs that pass FP
> values as
> > wider integers.
> > 
> > On Mon, 14 Mar 2022, Roger Sayle wrote:
> > 
> > >
> > > Hi Richard,
> > > Thanks for asking.
> > > The issue is with 16-bit floating point HFmode, where clang on nvptx
> > > passes HFmode values in SImode registers, and for binary compatibility
> > > GCC needs to do the same.
> > > Their motivation is that for compatibility with older GPUs and the
> > > x86_64 host, HFmode parameters are treated like "unsigned int".
> > > Hence, libgcc functions like __sqrthf behave as though declared (are
> > > binary compatible with) "unsigned short __sqrthf(unsigned short)".
> > > As you point out, this also greatly simplifies soft-float, as both
> > > ABIs remain the same.
> > >
> > > Alas, the subreg approach doesn't work as we'd end up with (subreg:SI
> > > (subreg:HI (reg:HF)), though technically that is what this patch does,
> > > inserting a pair of conversion instructions.
> > 
> > So what does FUNCTION_ARG return for the HFmode parameters?  The above
> > seems backwards - it should be (subreg:HF (reg:SI 0)), no?
> > 
> > Or are we talking about the caller side?  Or the return value case (in
> which case I
> > ask the same question wrt FUNCTION_VALUE)?
> > 
> > Does nvptx use the promotion hooks in those cases?
> > 
> > > Until recently (subreg:SI (reg:HF)) did work, but that support was
> > > removed/cleaned-up,
> > > as we need sensible subreg semantics in the RTL passes.   The proposed
> patch
> > > simply
> > > adds support back in the minimal places where changing FP/non-FP and
> > > precision at the same time is required: argument passing and return
> > > values.
> > >
> > > Hopefully this answers your question.  An alternate viewpoint might be
> > > "is there a reason for GCC not to be able to support targets with
> > > slightly wacky parameter passing conventions".
> > >
> > > Thanks for thinking about this.
> > > Roger
> > > --
> > >
> > > > -----Original Message-----
> > > > From: Richard Biener <rguenther@suse.de>
> > > > Sent: 14 March 2022 09:09
> > > > To: Roger Sayle <roger@nextmovesoftware.com>
> > > > Cc: 'Tom de Vries' <tdevries@suse.de>; 'Jeff Law'
> > > > <jeffreyalaw@gmail.com>; 'Tobias Burnus' <tobias@codesourcery.com>;
> > > > richard.sandiford@arm.com;
> > > 'Jeff
> > > > Law' <law@redhat.com>; gcc-patches@gcc.gnu.org
> > > > Subject: RE: PING**4 - [PATCH] middle-end: Support ABIs that pass FP
> > > values as
> > > > wider integers.
> > > >
> > > > On Mon, 14 Mar 2022, Roger Sayle wrote:
> > > >
> > > > >
> > > > > 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.
> > > >
> > > > First of all I'm not familiar with the ABI related code as all, so I
> > > refrained from
> > > > commenting.  But now I've looked closer (still unfamiliar code).
> > > >
> > > > I suppose there's targets passing SFmode in a SImode GPR and DFmode
> > > > in a DImode GPR (all soft-float targets?), so that already works
> somehow.
> > > > Why does nvptx choose DImode for SFmode passing then?  Can't it
> > > > choose (subreg:SI di:..) or so?  Does it require zero-extending to
> > > > DImode on the
> > > caller
> > > > side?  It seems your expand_expr_real_1 code does not rely on that?
> > > > So,
> > > why
> > > > does nvptx function_arg hook (?) insist on returning a DImode reg
> > > > for an
> > > SFmode
> > > > argument rather than an SImode subreg of that?
> > > >
> > > > Richard.
> > > >
> > > > >
> > > > > 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
> > > > >
> > > >
> > > > --
> > > > Richard Biener <rguenther@suse.de>
> > > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409
> > > > Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
> > >
> > >
> > 
> > --
> > Richard Biener <rguenther@suse.de>
> > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> > Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)