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 <tdevr...@suse.de> > > Sent: 14 March 2022 08:06 > > To: Jeff Law <jeffreya...@gmail.com>; Richard Biener <rguent...@suse.de>; > > Tobias Burnus <tob...@codesourcery.com> > > Cc: richard.sandif...@arm.com; Jeff Law <l...@redhat.com>; gcc- > > patc...@gcc.gnu.org; Roger Sayle <ro...@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 <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
