Tamar Christina <tamar.christ...@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
