Tamar Christina <tamar.christ...@arm.com> writes:
> Hi All,
>
> Currently some target supports checks such as vect_int cache their
> results in a manner that would cause them not to be rechecked when
> running the same tests against a different variant in a multi variant
> run. This causes tests to be skipped or run when they shouldn't be.
>
> there is already an existing caching mechanism in place that does the
> caching correctly, but presumably these weren't used because some of
> these tests originally only contained static data. e.g. only checked
> if the target is aarch64*-*-* etc.
>
> This patch changes every function that needs to do any caching at all
> to use check_cached_effective_target which will cache per variant
> instead of globally.
Thanks for doing this!
> For those tests that already parameterize over et_index I have created
> check_cached_effective_target_indexed to handle this common case by
> creating a list containing the property name and the current value of
> et_index.
>
> These changes result in a much simpler implementation for most tests
> and a large reduction in lines for target-supports.exp.
>
> Regtested on
> aarch64-none-elf
> x86_64-pc-linux-gnu
> powerpc64-unknown-linux-gnu
> arm-none-eabi
>
> and no testsuite errors. Difference would depend on your site.exp.
> On arm we get about 4500 new testcases and on aarch64 the low 10s.
> On PowerPC and x86_64 no changes as expected since the default exp for these
> just test the default configuration.
Would be good to try --target_board unix{,-m32} on x86_64.
> What this means for new target checks is that they should always use either
> check_cached_effective_target or check_cached_effective_target_indexed if the
> result of the check is to be cached.
>
> As an example the new vect_int looks like
>
> proc check_effective_target_vect_int { } {
> return [check_cached_effective_target_indexed <name> {
> expr {
> <condition>
> }}]
> }
>
> The debug information that was once there is now all hidden in
> check_cached_effective_target, (called from
> check_cached_effective_target_indexed) and so the only thing you are
> required to do is give it a unique cache name and a condition.
>
> The condition doesn't need to be an if statement so simple boolean
> expressions are enough here:
>
> [istarget i?86-*-*] || [istarget x86_64-*-*]
> || ([istarget powerpc*-*-*]
> && ![istarget powerpc-*-linux*paired*])
> || ...
>
> The expr may not be required as check_cached_effective_target forces
> evaluation, but I have left these here just to be sure (TCL semantics
> is confusing at times.).
It's required, since "eval" just runs a tcl script (which can give back
an arbitrary string) while "expr" evaluates the string as an expression.
This means that the caching (as written) should work for arbitrary strings,
such as command-line flags or the default value of -march.
> @@ -120,22 +121,41 @@ proc check_cached_effective_target { prop args } {
> 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 {[string is true -strict $args] || [string is false -strict $args]} {
> + set et_cache($prop,$target) $args
> + } else {
> + set et_cache($prop,$target) [uplevel eval $args]
> + }
Why are the checks for true and false needed? We shouldn't be
using this function for something that's already true or false.
> +# Implements a version of check_cached_effective_target that also takes
> et_index
> +# into account when creating the key for the cache.
> +proc check_cached_effective_target_indexed { prop args } {
> + global et_index
> + set key "$et_index $prop"
> + verbose "check_cached_effective_target_index $prop: returning $key" 2
> +
> + # Force the evaluation at this level since check_cached_effective_target
> + # may no longer be able to.
> + if {[string is true -strict $args] || [string is false -strict $args]} {
> + set value $args
> + } else {
> + set value [uplevel eval $args]
> + }
> + return [check_cached_effective_target $key $value]
> +}
This defeats the point of the cache, since we'll evaluate the expression
every time before passing it to check_cached_effective_target.
How about:
return [check_cached_effective_target $key [list uplevel eval $args]]
> @@ -149,11 +169,13 @@ proc clear_effective_target_cache { } {
> global et_cache
> global et_prop_list
>
> + set target [current_target_name]
> if {[info exists et_prop_list]} {
> verbose "clear_effective_target_cache: $et_prop_list" 2
> foreach prop $et_prop_list {
> - unset et_cache($prop,value)
> - unset et_cache($prop,target)
> + if {[info exists et_cache($prop,$target)]} {
> + unset et_cache($prop,$target)
> + }
Looks a bit odd that we now have to make this conditional. Maybe we
should maintain an et_prop_list for each target. I.e.:
if {![info exists et_prop_list]
|| [lsearch $et_prop_list $prop] < 0} {
lappend et_prop_list $prop
}
in check_cached_effective_target would become an unconditional:
lappend et_prop_list($target) $prop
(since $prop should already be in the cache for $target if it's
already in the list) and clear_effective_target_cache would then use:
if {[info exists et_prop_list($target)]} {
verbose "clear_effective_target_cache: $et_prop_list($target)" 2
foreach prop $et_prop_list($target) {
unset et_cache($prop,$target)
}
unset et_prop_list($target)
}
...That said, I'm not sure what the point of et_prop_list actually is.
The comment above clear_effective_target_cache says:
# Clear effective-target cache. This is useful after testing
# effective-target features and overriding TEST_ALWAYS_FLAGS and/or
# ALWAYS_CXXFLAGS.
# If one changes ALWAYS_CXXFLAGS or TEST_ALWAYS_FLAGS then they should
# do a clear_effective_target_cache at the end as the target cache can
# make decisions based upon the flags, and those decisions need to be
# redone when the flags change. An example of this is the
# asan_init/asan_finish pair.
so at first I thought it was to avoid removing entries from before
TEST_ALWAYS_FLAGS & co. had been changed, since those entries should
still be valid after the flags have been reset. But that would only
happen if something cleared et_prop_list before changing TEST_ALWAYS_FLAGS
(so that et_prop_list only contains entries that postdate the change),
and AFAICT nothing does.
So I think we should simply remove et_prop_list and use an unconditional:
array unset et_cache
in clear_effective_target_cache.
> @@ -380,12 +402,9 @@ proc check_visibility_available { what_kind } {
> # be determined.
>
> proc check_alias_available { } {
> - global alias_available_saved
> global tool
>
> - if [info exists alias_available_saved] {
> - verbose "check_alias_available returning saved
> $alias_available_saved" 2
> - } else {
> + return [check_cached_effective_target alias_available {
> set src alias[pid].c
> set obj alias[pid].o
> verbose "check_alias_available compiling testfile $src" 2
> @@ -402,7 +421,7 @@ proc check_alias_available { } {
>
> if [string match "" $lines] then {
> # No error messages, everything is OK.
> - set alias_available_saved 2
> + return 2
> } else {
> if [regexp "alias definitions not supported" $lines] {
> verbose "check_alias_available target does not support
> aliases" 2
> @@ -411,24 +430,20 @@ proc check_alias_available { } {
>
> if { $objformat == "elf" } {
> verbose "check_alias_available but target uses ELF format,
> so it ought to" 2
> - set alias_available_saved -1
> + return -1
> } else {
> - set alias_available_saved 0
> + return 0
> }
> } else {
> if [regexp "only weak aliases are supported" $lines] {
> verbose "check_alias_available target supports only weak
> aliases" 2
> - set alias_available_saved 1
> + return 1
> } else {
> - set alias_available_saved -1
> + return -1
> }
> }
> }
> -
> - verbose "check_alias_available returning $alias_available_saved" 2
> - }
> -
> - return $alias_available_saved
> + }]
> }
As it stands, these should be "expr" rather than "return".
"return" will make check_cached_effective_target return at the point of
the uplevel eval, bypassing the actual caching.
If instead you want to support "return" (would be nice IMO), you could
use something like:
set code [catch {uplevel eval $args} result options]
if {$code != 0 && $code != 2} {
return -code $code $result
}
set et_cache($prop,$target) $result
Thanks,
Richard
