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