(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
