Re: [PATCH][GCC][testsuite] Fix caching of tests for multiple variant runs and update existing target-supports tests.

[Richard Sandiford <richard dot sandiford at arm dot com>]

Tamar Christina <Tamar.Christina@arm.com> writes:
>> >
>> > The error you would get if you do this is very confusing so I thought
>> > since it didn't matter much for the regexp only target triple tests
>> > that just accepting this would be fine.
>> 
>> Seems a good thing that that's a noisy failure; the function should
>> make up its mind whether it wants to cache (use curly braces) or not
>> (just return the expr directly).
>> 
>> > Should I drop it or keep it?
>> 
>> Think we should either drop it or make it into a more user-friendly
>> error, e.g.:
>> 
>> 	if {[string is true -strict $args] || [string is false -strict $args]} {
>> 	    error {check_cached_effective_target condition already evaluated; did you pass [...] instead of the expected {...}?}
>> 	} else {
>> 	    set et_cache($prop,$target) [uplevel eval $args]
>> 	}
>>
>
> Done, I have also ran a regression test on x86_64 with unix{,-m32} and
> no fallouts, testsuite is clean.
>
> Attached updated patch with feedback processed.
>
> Ok for trunk?

Looks good, some minor things below.

> @@ -117,25 +118,36 @@ proc current_target_name { } {
> 
>  proc check_cached_effective_target { prop args } {
>      global et_cache
> -    global et_prop_list
> 
>      set target [current_target_name]
> -    if {![info exists et_cache($prop,target)]
> -	|| $et_cache($prop,target) != $target} {
> +    if {![info exists et_cache($prop,$target)]} {
>  	verbose "check_cached_effective_target $prop: checking $target" 2
> -	set et_cache($prop,target) $target
> -	set et_cache($prop,value) [uplevel eval $args]
> -	if {![info exists et_prop_list]
> -	    || [lsearch $et_prop_list $prop] < 0} {
> -	    lappend et_prop_list $prop
> +	if {[string is true -strict $args] || [string is false -strict $args]} {
> +	    error {check_cached_effective_target condition already evaluated;\
> +		   did you pass [...] instead of the expected {...}?}

Should be no line break here, since the extra spaces will be part of the
error message.

> @@ -657,7 +646,8 @@ proc check_profiling_available { test_what } {
>      }
 
>      # Now examine the cache variable.
> -    if {![info exists profiling_available_saved]} {
> +    set profiling_working \
> +       [expr {[check_cached_effective_target profiling_available {
>  	# Some targets don't have any implementation of __bb_init_func or are
>  	# missing other needed machinery.
>  	if {[istarget aarch64*-*-elf]

No need for this [expr {...}] wrapper, can just use
[check_cached_effective_target profiling_available ...] directly.

> @@ -3072,12 +3057,10 @@ proc check_effective_target_vect_cmdline_needed { } {
>               || [istarget spu-*-*]
>  	     || ([istarget arm*-*-*] && [check_effective_target_arm_neon])
>  	     || [istarget aarch64*-*-*] } {
> -	   set et_vect_cmdline_needed_saved 0
> -	}
> -    }
> -
> -    verbose "check_effective_target_vect_cmdline_needed: returning $et_vect_cmdline_needed_saved" 2
> -    return $et_vect_cmdline_needed_saved
> +		return 0
> +	} else {
> +		return 1
> +	}}]
>  }

Returns indented too far.

> @@ -5720,17 +5479,7 @@ proc check_effective_target_vect_no_int_add { } {
>  # This won't change for different subtargets so cache the result.
 
>  proc check_effective_target_vect_no_bitwise { } {
> -    global et_vect_no_bitwise_saved
> -    global et_index
> -
> -    if [info exists et_vect_no_bitwise_saved($et_index)] {
> -	verbose "check_effective_target_vect_no_bitwise: using cached result" 2
> -    } else {
> -	set et_vect_no_bitwise_saved($et_index) 0
> -    }
> -    verbose "check_effective_target_vect_no_bitwise:\
> -	     returning $et_vect_no_bitwise_saved($et_index)" 2
> -    return $et_vect_no_bitwise_saved($et_index)
> +    return [check_cached_effective_target_indexed vect_no_bitwise { expr 0 }]
>  }

Might as well just return 0 here.  But OK if you want to keep it.

> @@ -6523,20 +5978,15 @@ proc check_effective_target_vect_aligned_arrays { } {
>  # This won't change for different subtargets so cache the result.
> 
>  proc check_effective_target_natural_alignment_32 { } {
> -    global et_natural_alignment_32
> -
> -    if [info exists et_natural_alignment_32_saved] {
> -        verbose "check_effective_target_natural_alignment_32: using cached result" 2
> -    } else {
> -        # FIXME: 32bit powerpc: guaranteed only if MASK_ALIGN_NATURAL/POWER.
> -        set et_natural_alignment_32_saved 1
> -        if { ([istarget *-*-darwin*] && [is-effective-target lp64])
> -             || [istarget avr-*-*] } {
> -            set et_natural_alignment_32_saved 0
> -        }
> -    }
> -    verbose "check_effective_target_natural_alignment_32: returning $et_natural_alignment_32_saved" 2
> -    return $et_natural_alignment_32_saved
> +    # FIXME: 32bit powerpc: guaranteed only if MASK_ALIGN_NATURAL/POWER.
> +    return [check_cached_effective_target_indexed natural_alignment_32 {
> +	    if { ([istarget *-*-darwin*] && [is-effective-target lp64])
> +		  || [istarget avr-*-*] } {
> +		return 0
> +	    } else {
> +		return 1
> +	    }
> +    }]
>  }
 
Excess indentation of { ... } body.

>  # Return 1 if types of size 64 bit or less are naturally aligned (aligned to their
> @@ -6545,19 +5995,10 @@ proc check_effective_target_natural_alignment_32 { } {
>  # This won't change for different subtargets so cache the result.
 
>  proc check_effective_target_natural_alignment_64 { } {
> -    global et_natural_alignment_64
> -
> -    if [info exists et_natural_alignment_64_saved] {
> -        verbose "check_effective_target_natural_alignment_64: using cached result" 2
> -    } else {
> -        set et_natural_alignment_64_saved 0
> -        if { ([is-effective-target lp64] && ![istarget *-*-darwin*])
> -             || [istarget spu-*-*] } {
> -            set et_natural_alignment_64_saved 1
> -        }
> -    }
> -    verbose "check_effective_target_natural_alignment_64: returning $et_natural_alignment_64_saved" 2
> -    return $et_natural_alignment_64_saved
> +    return [check_cached_effective_target_indexed natural_alignment_64 {
> +	    expr { ([is-effective-target lp64] && ![istarget *-*-darwin*])
> +		  || [istarget spu-*-*] }
> +    }]
>  }

Same here.

> @@ -6659,21 +6086,10 @@ proc check_effective_target_vect_unaligned_possible { } {
>  # Return 1 if the target supports vector LOAD_LANES operations, 0 otherwise.
 
>  proc check_effective_target_vect_load_lanes { } {
> -    global et_vect_load_lanes
> -
> -    if [info exists et_vect_load_lanes] {
> -	verbose "check_effective_target_vect_load_lanes: using cached result" 2
> -    } else {
> -	set et_vect_load_lanes 0
> -	# We don't support load_lanes correctly on big-endian arm.
> -	if { ([check_effective_target_arm_little_endian] && [check_effective_target_arm_neon_ok])
> -	     || [istarget aarch64*-*-*] } {
> -	    set et_vect_load_lanes 1
> -	}
> -    }
> -
> -    verbose "check_effective_target_vect_load_lanes: returning $et_vect_load_lanes" 2
> -    return $et_vect_load_lanes
> +    # We don't support load_lanes correctly on big-endian arm.
> +    return [check_cached_effective_target vect_load_lanes {
> +      expr { ([check_effective_target_arm_little_endian] && [check_effective_target_arm_neon_ok])
> +	     || [istarget aarch64*-*-*] }}]
>  }

Long line.
 
> @@ -7687,8 +6760,17 @@ proc is-effective-target { arg } {
>  	  default          { error "unknown effective target keyword `$arg'" }
>  	}
>      }
> +
>      verbose "is-effective-target: $arg $selected" 2
> -    return $selected
> +    if {[string is true -strict $selected]
> +	|| [string is false -strict $selected]} {
> +	set result $selected
> +    } else {
> +	set result [uplevel eval $selected]
> +	verbose "is-effective-target forced eval: $arg $result" 2
> +    }
> +
> +    return $result
>  }

Is this necessary?  $selected is set by things like:

	  "vmx_hw"         { set selected [check_vmx_hw_available] }

so should already be correct.

> @@ -8817,23 +7899,17 @@ proc check_effective_target_fenv_exceptions {} {
>  }
 
>  proc check_effective_target_tiny {} {
> -    global et_target_tiny_saved
> -
> -    if [info exists et_target_tiny_saved] {
> -      verbose "check_effective_target_tiny: using cached result" 2
> -    } else {
> -       set et_target_tiny_saved 0
> -       if { [istarget aarch64*-*-*]
> -             && [check_effective_target_aarch64_tiny] } {
> -         set et_target_tiny_saved 1
> -       }
> -       if { [istarget avr-*-*]
> -             && [check_effective_target_avr_tiny] } {
> -         set et_target_tiny_saved 1
> -       }
> -    }
> -
> -    return $et_target_tiny_saved
> +    return [check_cached_effective_target tiny {
> +	    if { [istarget aarch64*-*-*]
> +		 && [check_effective_target_aarch64_tiny] } {
> +	      return 1
> +	    }
> +	    if { [istarget avr-*-*]
> +		 && [check_effective_target_avr_tiny] } {
> +	      return 1
> +	    }
> +	    return 0
> +    }]
>  }

Over-indented { ... } body.
 
OK with those changes if you can drop the is-effective-target hunk above.

Thanks,
Richard