Re: using scratchpads to enhance RTL-level if-conversion: revised patch

[Bernd Schmidt <bschmidt at redhat dot com>]

(Jakub Cc'd because of code he added for PR23567).

On 10/27/2015 11:35 PM, Abe wrote:
> Thanks for all your feedback.  I have integrated as much of it as I
> could in the available time.

Unfortunately not all of it - I still think we need to have a better 
strategy of selecting a scratchpad than a newly allocated stack slot. 
There are sufficiently many options.

>          * ifcvt.c (noce_mem_write_may_go_wrong_even_with_scratchpads):
> New.

ChangeLog doesn't correspond to the patch. If the function actually 
existed I'd reject it for a way overlong identifier.

>          * target.h: New enum named "RTL_ifcvt_when_to_use_scratchpads".

Please follow ChangeLog writing guidelines.

> -/* Return true if a write into MEM may trap or fault.  */
> +/* Return true if a write into MEM may trap or fault
> +   even in the presence of scratchpad support.  */

I still think this comment is fairly useless and needs to better 
describe what it is actually doing.

>   static bool
> -noce_mem_write_may_trap_or_fault_p (const_rtx mem)
> +noce_mem_write_may_trap_or_fault_p_1 (const_rtx mem)

For what this ends up doing, I think a name like "unsafe_address_p" 
would be better. Also, I think the code in there is really dubious - it 
tries to look for SYMBOL_REFs which are in decl_readonly_section, but 
shouldn't that be unnecessary given the test for MEM_READONLY_P? It's 
completely unreliable anyway since the address could be loaded into a 
register (on most targets it would be) and we'd never see the SYMBOL_REF 
in that function.

> +/* Return true if a write into MEM may trap or fault
> +   without scratchpad support.  */

Just keep the previous comment without mentioning scratchpads.

> +static bool
> +noce_mem_write_may_trap_or_fault_p (const_rtx mem)
> +{
> +  if (may_trap_or_fault_p (mem))
> +    return true;
> +
> +  return noce_mem_write_may_trap_or_fault_p_1 (mem);
> +}
> +      if (RTL_ifcvt_use_spads_as_per_profile
> +        == targetm.RTL_ifcvt_when_to_use_scratchpads
> +          && (PROFILE_ABSENT == profile_status_for_fn (cfun)
> +          || PROFILE_GUESSED == profile_status_for_fn (cfun)
> +          || predictable_edge_p (then_edge)
> +          || ! maybe_hot_bb_p (cfun, then_bb)))
> +        return FALSE;

I guess this is slightly better than no cost estimate at all.

> +      if (noce_mem_write_may_trap_or_fault_p_1 (orig_x)

So why do you want to call this function anyway? Doesn't the scratchpad 
technique protect against storing to a bad address?

> +      const size_t MEM_size = MEM_SIZE (orig_x);

No uppercase letters for variables and such. Just use "sz" or "size" for 
brevity.

> +          biggest_spad = assign_stack_local (GET_MODE (orig_x),

Still the same problems - this is the least attractive choice of a 
scratchpad location, and the code may end up allocating more scratchpads 
than you need.

> +      emit_insn_before_setloc (seq, if_info->jump,
> +                   INSN_LOCATION (if_info->insn_a));

Formatting? Could be mail client damage.

> +DEFHOOKPOD
> +(RTL_ifcvt_when_to_use_scratchpads,
> +"*",
> +enum RTL_ifcvt_when_to_use_scratchpads,
> RTL_ifcvt_use_spads_as_per_profile)
> +

No caps, and maybe a less clumsy name.

> +enum RTL_ifcvt_when_to_use_scratchpads {
> +  RTL_ifcvt_never_use_scratchpads = 0,
> +  RTL_ifcvt_always_use_scratchpads,
> +  RTL_ifcvt_use_spads_as_per_profile
> +};

Likewise. Maybe

enum ifcvt_scratchpads_strategy {
  scratchpad_never,
  scratchpad_always,
  scratchpad_unpredictable
};

So, still a NACK from my side.


Bernd