Re: improved RTL-level if conversion using scratchpads [half-hammock edition]

[Bernd Schmidt <bschmidt at redhat dot com>]

On 11/06/2015 12:43 AM, Abe wrote:
> Feedback from Bernd has also been applied.

But inconsistently, and I think not quite in the way I meant it in one case.

> -/* Return true if a write into MEM may trap or fault.  */
> -
>   static bool
>   noce_mem_write_may_trap_or_fault_p (const_rtx mem)
>   {
> -  rtx addr;
> -
>     if (MEM_READONLY_P (mem))
>       return true;
>
> -  if (may_trap_or_fault_p (mem))
> -    return true;
> -
> +  rtx addr;
>     addr = XEXP (mem, 0);
>
>     /* Call target hook to avoid the effects of -fpic etc....  */
> @@ -2881,6 +2883,18 @@ noce_mem_write_may_trap_or_fault_p (const_rtx mem)
>     return false;
>   }
>
> +/* Return true if a write into MEM may trap or fault
> +   without scratchpad support.  */
> +
> +static bool
> +unsafe_address_p (const_rtx mem)
> +{
> +  if (may_trap_or_fault_p (mem))
> +    return true;
> +
> +  return noce_mem_write_may_trap_or_fault_p (mem);
> +}

The naming seems backwards from what I suggested in terms of naming. You 
still haven't explained why you want to modify this function, or call 
the limited one even when generating scratchpads. I asked about this 
last time.

>   static basic_block
> -find_if_header (basic_block test_bb, int pass)
> +find_if_header (basic_block test_bb, int pass, bool just_sz_spad)
>   {

Arguments need documentation.

> +DEFPARAM (PARAM_FORCE_ENABLE_RTL_IFCVT_SPADS,
> +	  "force-enable-rtl-ifcvt-spads",
> +	  "Force-enable the use of scratchpads in RTL if conversion, "
> +	  "overriding the target and the profile data or lack thereof.",
> +	  0, 0, 1)
> +
>
> +DEFHOOKPOD
> +(rtl_ifcvt_scratchpad_control,
> +"*",
> +enum rtl_ifcvt_spads_ctl_enum, rtl_ifcvt_spads_as_per_profile)
> +
> +DEFHOOK
> +(rtl_ifcvt_get_spad,
> + "*",
> + rtx, (unsigned short size),
> + default_rtl_ifcvt_get_spad)

That moves the problematic bit in a target hook rather than fixing it. 
Two target hooks and a param is probably a bit much for a change like 
this. Which target do you actually want this for? It would help to 
understand why you're doing all this.

> +enum rtl_ifcvt_spads_ctl_enum {

Names are still too verbose ("enum" shouldn't be part of it).

> +rtx default_rtl_ifcvt_get_spad (unsigned short size)
> +{
> +  return assign_stack_local (BLKmode, size, 0);
> +}

Formatting problem, here and in a few other places. I didn't fully read 
the patch this time around.

I'm probably not reviewing further patches because I don't see this 
progressing to a state where it's acceptable. Others may do so, but as 
far as I'm concerned the patch is rejected.


Bernd