| Message ID | 15881f44-1bed-4fda-a47c-45234f9c091e@BAMAIL02.ba.imgtec.org |
|---|---|
| State | New |
| Headers | show |
On 11/06/2015 08:04 PM, Steve Ellcey wrote: > When I made this change I had one regression in the GCC testsuite > (gcc.dg/fixed-point/convert-sat.c). I tracked this down to the > fact that emit_library_call_value_1 does not do any argument promotion > because it does not have the original tree type information for library > calls. It only knows about modes. I can't change emit_library_call_value_1 > to do the promotion because it does not know whether to do a signed or > unsigned promotion, but expand_fixed_convert could do the conversion > before calling emit_library_call_value_1 and that is what this patch does. Hmm, difficult. I can see how there would be a problem, but considering how many calls to emit_library_call_* we have, I'm slightly worried whether this is really is a good approach. On the other hand, there seems to be precedent in this file: if (GET_MODE_PRECISION (GET_MODE (from)) < GET_MODE_PRECISION (SImode)) from = convert_to_mode (SImode, from, unsignedp); > The 'real' long term fix for this problem is to have tree types for builtin > functions so the proper promotions can always be done but that is a fairly > large change that I am not willing to tackle right now and it could probably > not be done in time for GCC 6.0 anyway. Yeah, but I agree that this is the real fix. We should aim to get rid of the emit_library_call functions. > + if (SCALAR_INT_MODE_P (from_mode)) > + { > + /* If we need to promote the integer function argument we need to do Extra space at the start of the comment. > + it here instead of inside emit_library_call_value because here we > + know if we should be doing a signed or unsigned promotion. */ > + > + machine_mode arg_mode; > + int unsigned_p = 0; > + > + arg_mode = promote_function_mode (NULL_TREE, from_mode, > + &unsigned_p, NULL_TREE, 0); > + if (arg_mode != from_mode) > + { > + from = convert_to_mode (arg_mode, from, uintp); > + from_mode = arg_mode; > + } > + } Move this into a separate function (prepare_libcall_arg)? I'll think about it over the weekend and let others chime in if they want, but I think I'll probably end up approving it with that change. Bernd
On Fri, 2015-11-06 at 20:14 +0100, Bernd Schmidt wrote: > > > + if (SCALAR_INT_MODE_P (from_mode)) > > + { > > + /* If we need to promote the integer function argument we need to do > > Extra space at the start of the comment. > > > + it here instead of inside emit_library_call_value because here we > > + know if we should be doing a signed or unsigned promotion. */ > > + > > + machine_mode arg_mode; > > + int unsigned_p = 0; > > + > > + arg_mode = promote_function_mode (NULL_TREE, from_mode, > > + &unsigned_p, NULL_TREE, 0); > > + if (arg_mode != from_mode) > > + { > > + from = convert_to_mode (arg_mode, from, uintp); > > + from_mode = arg_mode; > > + } > > + } > > Move this into a separate function (prepare_libcall_arg)? I'll think > about it over the weekend and let others chime in if they want, but I > think I'll probably end up approving it with that change. > > > Bernd Are you thinking of a simple function that is called on all targets or a target specific function? Maybe a target specific function would be safer. We could have: from = targetm.prepare_libcall_arg (from, uintp); from_mode = GET_MODE (from); And have the default prepare_libcall_arg simply return the original 'from' rtx value. We could also pass some other arguments, maybe the library call rtx (fun) and an integer to specify what argument this is (1, 2, 3, ...) if we wanted to try and generalize it even more. Steve Ellcey sellcey@imgtec.com
On 11/06/2015 08:27 PM, Steve Ellcey wrote: > > Are you thinking of a simple function that is called on all targets or a > target specific function? Maybe a target specific function would be > safer. No, I think just what you have there is probably sufficient. Bernd
On Fri, Nov 6, 2015 at 8:14 PM, Bernd Schmidt <bschmidt@redhat.com> wrote: > On 11/06/2015 08:04 PM, Steve Ellcey wrote: >> >> When I made this change I had one regression in the GCC testsuite >> (gcc.dg/fixed-point/convert-sat.c). I tracked this down to the >> fact that emit_library_call_value_1 does not do any argument promotion >> because it does not have the original tree type information for library >> calls. It only knows about modes. I can't change >> emit_library_call_value_1 >> to do the promotion because it does not know whether to do a signed or >> unsigned promotion, but expand_fixed_convert could do the conversion >> before calling emit_library_call_value_1 and that is what this patch does. > > > Hmm, difficult. I can see how there would be a problem, but considering how > many calls to emit_library_call_* we have, I'm slightly worried whether this > is really is a good approach. > > On the other hand, there seems to be precedent in this file: > > if (GET_MODE_PRECISION (GET_MODE (from)) < GET_MODE_PRECISION > (SImode)) > from = convert_to_mode (SImode, from, unsignedp); > >> The 'real' long term fix for this problem is to have tree types for >> builtin >> functions so the proper promotions can always be done but that is a fairly >> large change that I am not willing to tackle right now and it could >> probably >> not be done in time for GCC 6.0 anyway. > > > Yeah, but I agree that this is the real fix. We should aim to get rid of the > emit_library_call functions. Indeed. In the "great plan" of simplifying RTL expansion by moving stuff up to the GIMPLE level this could be done in a lowering stage lowering all operations that we need to do via libcalls to GIMPLE calls. Now, we'd either need proper function declarations for all libcalls of optabs for this or have the optab internal function stuff from Richard also provide the libcall fallback. In the expansion code for the as-libcall path we can then simply use the type of the incoming argument (as we could if emit_library_call_value_1 would in addition to the RTX operands also receive the original tree ones). Richard. >> + if (SCALAR_INT_MODE_P (from_mode)) >> + { >> + /* If we need to promote the integer function argument we need to >> do > > > Extra space at the start of the comment. > >> + it here instead of inside emit_library_call_value because here >> we >> + know if we should be doing a signed or unsigned promotion. */ >> + >> + machine_mode arg_mode; >> + int unsigned_p = 0; >> + >> + arg_mode = promote_function_mode (NULL_TREE, from_mode, >> + &unsigned_p, NULL_TREE, 0); >> + if (arg_mode != from_mode) >> + { >> + from = convert_to_mode (arg_mode, from, uintp); >> + from_mode = arg_mode; >> + } >> + } > > > Move this into a separate function (prepare_libcall_arg)? I'll think about > it over the weekend and let others chime in if they want, but I think I'll > probably end up approving it with that change. > > > Bernd
diff --git a/gcc/optabs.c b/gcc/optabs.c index fdcdc6a..964e92a 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -4880,6 +4880,24 @@ expand_fixed_convert (rtx to, rtx from, int uintp, int satp) libfunc = convert_optab_libfunc (tab, to_mode, from_mode); gcc_assert (libfunc); + if (SCALAR_INT_MODE_P (from_mode)) + { + /* If we need to promote the integer function argument we need to do + it here instead of inside emit_library_call_value because here we + know if we should be doing a signed or unsigned promotion. */ + + machine_mode arg_mode; + int unsigned_p = 0; + + arg_mode = promote_function_mode (NULL_TREE, from_mode, + &unsigned_p, NULL_TREE, 0); + if (arg_mode != from_mode) + { + from = convert_to_mode (arg_mode, from, uintp); + from_mode = arg_mode; + } + } + start_sequence (); value = emit_library_call_value (libfunc, NULL_RTX, LCT_CONST, to_mode, 1, from, from_mode);