RISCV test infrastructure for d / v / zfh extensions

  This makes it easier to write tests that safely test features needing
d, v and/or zfh extensions.

check_effective_target_riscv_v checks if the current target allows to
use vector instructions.
add_options_for_riscv_v ask to add an -arch option to change the
target to one like the current one, but with the 'v' extension
enabled, if it is not already is.  That is generally safe for
compile-only tests, e.g. using scan-assembler* stanzas.
If you have an execution test that you want to force usin the
extension if the actual execution
target supports that, you can use check_effective_target_riscv_v_ok to
check if that's ok, and then
add_options_for_riscv_v to add the appropriate -march option.

 Examples how this can be used can be found
athttps://github.com/embecosm/rvv-gcc/tree/rvv-12/gcc/testsuite
2023-04-17  Joern Rennecke  <joern.rennecke@embecosm.com>

gcc/testsuite/
	* lib/target-supports.exp (check_effective_target_rv_float_abi_soft):
	New proc.
	(check_effective_target_riscv_d): Likewise.
	(check_effective_target_riscv_v): Likewise.
	(check_effective_target_riscv_zfh): Likewise.
	(check_effective_target_riscv_v_ok): likewise.
	(check_effective_target_riscv_zfh_ok): Likewise.
	(riscv_get_arch, add_options_for_riscv_v): Likewise.
	(add_options_for_riscv_zfh): Likewise.
	(add_options_for_riscv_d): Likewise.
  

Hi Joern,

thanks, I believe this will help with testing.

> +proc check_effective_target_riscv_v { } {
> +    return [check_no_compiler_messages riscv_ext_v assembly {
> +       #ifndef __riscv_v
> +       #error "Not __riscv_v"
> +       #endif
> +    }]
> +}
This can be replaced by riscv_vector or vice versa.

> +# Return 1 if we can execute code when using dg-add-options riscv_v
> +
> +proc check_effective_target_riscv_v_ok { } {
> +    # If the target already supports v without any added options,
> +    # we may assume we can execute just fine.
> +    if { [check_effective_target_riscv_v] } {
> +	return 1
> +    }
> +
> +    # check if we can execute vector insns with the given hardware or
> +    # simulator
> +    set gcc_march [regsub {[[:alnum:]]*} [riscv_get_arch] &v]
> +    if { [check_runtime ${gcc_march}_exec {
> +	  int main() {  asm("vsetivli t0, 9, e8, m1, tu, ma"); return 0; } } "-march=${gcc_march}"] } {
> +	return 1
> +    }
> +
> +    # Possible future extensions: If the target is a simulator, dg-add-options
> +    # might change its config to make it allow vector insns, or we might use
> +    # options to set special elf flags / sections to effect that.
> +
> +    return 0
> +}
So in general we would add {dg-add-options riscv_v} for every
test that requires compile-time vector support?

For a run test we would check {dg-require-effective-target riscv_v_ok}
before?

Would it make sense to skip the first check here
(check_effective_target_riscv_v) so we have a proper runtime check?
Right now we assume the runtime can execute vector instructions if
the compiler can emit them.  You could replace riscv_vector_hw and
riscv_zvfh_hw by your versions then and we'd have a clear separation
between runtime and compile time.
We would just need to make sure not to add "v" twice if it's already
in the march string.

> +    if { [string equal $gcc_march "imafd"] } {
> +	set gcc_march "g"
> +    }
Wouldn't we want to always replace "imafd" with "g" for
simplicity/consistency and not just the exact string? 

> +proc add_options_for_riscv_v { flags } {
> +    if { [lsearch $flags -march=*] >= 0 } {
> +	# If there are multiple -march flags, we have to adjust all of them.
> +	# ??? Is there a way to make the match specific to a full list element?
> +	#     as it is, we might match something inside a string.
> +	return [regsub -all -- {(-march=rv[[:digit:]]*[a-rt-uwy]*)v*} $flags \\1v ]

Is iterating over the list elements and returning a new list
not an option?  Or would that break something else?

Regards
 Robin
  

On Tue, 1 Aug 2023 at 14:44, Robin Dapp <rdapp.gcc@gmail.com> wrote:
>
> Hi Joern,
>
> thanks, I believe this will help with testing.
>
> > +proc check_effective_target_riscv_v { } {
> > +    return [check_no_compiler_messages riscv_ext_v assembly {
> > +       #ifndef __riscv_v
> > +       #error "Not __riscv_v"
> > +       #endif
> > +    }]
> > +}
> This can be replaced by riscv_vector or vice versa.

Hmm, you are right.  I personally prefer my version because it allows
consistent naming of the
different tests, also easily extendible when new extensions need testing.
Although the riscv_vector name has the advantage that it is better
legible for people who are
not used to dealing with RISC_V extension names.  If we keep
riscv_vector, it would make
sense to name the other tests also something more verbose, e.g. change
riscv_d into
riscv_double_fp or even riscv_double_precision_floating_point .
It would be nice to hear other people's opinions on the naming.

> > +# Return 1 if we can execute code when using dg-add-options riscv_v
> > +
> > +proc check_effective_target_riscv_v_ok { } {
> > +    # If the target already supports v without any added options,
> > +    # we may assume we can execute just fine.
> > +    if { [check_effective_target_riscv_v] } {
> > +     return 1
> > +    }
> > +
> > +    # check if we can execute vector insns with the given hardware or
> > +    # simulator
> > +    set gcc_march [regsub {[[:alnum:]]*} [riscv_get_arch] &v]
> > +    if { [check_runtime ${gcc_march}_exec {
> > +       int main() {  asm("vsetivli t0, 9, e8, m1, tu, ma"); return 0; } } "-march=${gcc_march}"] } {
> > +     return 1
> > +    }
> > +
> > +    # Possible future extensions: If the target is a simulator, dg-add-options
> > +    # might change its config to make it allow vector insns, or we might use
> > +    # options to set special elf flags / sections to effect that.
> > +
> > +    return 0
> > +}
> So in general we would add {dg-add-options riscv_v} for every
> test that requires compile-time vector support?
>
> For a run test we would check {dg-require-effective-target riscv_v_ok}
> before?

Yes.

> Would it make sense to skip the first check here
> (check_effective_target_riscv_v) so we have a proper runtime check?

My starting point was that the changing of global testsuite variables around -
as the original RISC-V vector patches did - is wrong.  The user asked to test
a particular target (or set targets, for multilibs), and that target
is the one to test,
so we can't just assume it has other hardware features that are not implied by
the target.
Contrarily, the target that the user requested to test can be assumed to be
available for testing.  Testing that it actually works is a part of
the point of the
test.  If I ask for a dejagnu test for a target that has vector support, I would
hope that the vector support is also tested, not backing off if it finds that
there is a problem with the target,

Although it should get tested anyway with generic tests, even though this
does not happen at the moment (this is for another PR I intend to open
and address).

> Right now we assume the runtime can execute vector instructions if
> the compiler can emit them.

The way I look at things, when the macro  __riscv_v is defined,
the compiler asserts that it is compiling for a target that has vector support,
because it was instructed by configuration / options to emit code for that
target.  Which we can take as evidence that dejagnu is run with options
to select that target (either explicitly or by default due to the
configuration of
the compiler under test)

>  You could replace riscv_vector_hw and
> riscv_zvfh_hw by your versions then and we'd have a clear separation
> between runtime and compile time.
> We would just need to make sure not to add "v" twice if it's already
> in the march string.

The check_effective_target_riscv_v test (or riscv_vector of that name
is preferred)
in add_options_for_riscv_v would be unaffected by such a change.
This is purely a matter of what failure mode people want to see if the
execution target can't execute programs for the specified target.  Do
we want it to be a silent side-note in the verbose log file, or complain
for every single test?

> > +    if { [string equal $gcc_march "imafd"] } {
> > +     set gcc_march "g"
> > +    }
> Wouldn't we want to always replace "imafd" with "g" for
> simplicity/consistency and not just the exact string?

The tests are arranged such that the exact string "imafd" will appear
there if that is supported.  Note that the test is inside the loop.

I see that the indentation looks garbled, I got to fix that.

> > +proc add_options_for_riscv_v { flags } {
> > +    if { [lsearch $flags -march=*] >= 0 } {
> > +     # If there are multiple -march flags, we have to adjust all of them.
> > +     # ??? Is there a way to make the match specific to a full list element?
> > +     #     as it is, we might match something inside a string.
> > +     return [regsub -all -- {(-march=rv[[:digit:]]*[a-rt-uwy]*)v*} $flags \\1v ]
>
> Is iterating over the list elements and returning a new list
> not an option?  Or would that break something else?

I was afraid making it overly complex increases the likelihood of a
coding error,
and hoping there'd be some simple solution to request a list element start/end
that I wasn't aware of.

Hmm, come to think of it, even if I can't exactly match a list element, I could
match a list start or delimiter.  I just have to make sure i put the delimiter I
matched back.  And I can't match it as a regexp part at the end too, although
I could match it with the positive look ahead pattern; but I don't actually want
to match the end.
So we can make sure we are dealing with a list element
that looks like a -march option.  (You could still construe a multi-word option
that uses a string starting with -march as a pathname or similar, but I suppose
you'd deserve whatever you get then.  I don't see a bobby tables scenario
here.)

I also found one comment pasto.

I have attached the amended patch - not tested yet.  I hope to get some opinions
today on the patch call regarding the test naming and the behaviour of
dg-require-effective-target riscv_v_ok when testing an architecture variant with
vector support, so the idea is to test after any input from that is taken into
account, and also maybe in the context of the cpymem patch.
2023-08-15  Joern Rennecke  <joern.rennecke@embecosm.com>

gcc/testsuite/
	* lib/target-supports.exp (check_effective_target_rv_float_abi_soft):
	New proc.
	(check_effective_target_riscv_d): Likewise.
	(check_effective_target_riscv_v): Likewise.
	(check_effective_target_riscv_zfh): Likewise.
	(check_effective_target_riscv_v_ok): likewise.
	(check_effective_target_riscv_zfh_ok): Likewise.
	(riscv_get_arch, add_options_for_riscv_v): Likewise.
	(add_options_for_riscv_zfh): Likewise.
	(add_options_for_riscv_d): Likewise.

diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 92b6f69730e..cdd00b4a064 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -1887,6 +1887,167 @@ proc check_effective_target_rv64 { } {
     }]
 }
 
+# Return 1 if the target abi is __riscv_float_abi_soft, 0 otherwise.
+# Cache the result.
+
+proc check_effective_target_rv_float_abi_soft { } {
+    # Check that we are compiling for RV64 by checking the xlen size.
+    return [check_no_compiler_messages riscv_riscv_float_abi_soft assembly {
+       #ifndef __riscv_float_abi_soft
+       #error "Not __riscv_float_abi_soft"
+       #endif
+    }]
+}
+
+# Return 1 if the target arch supports the double precision floating point
+# extension, 0 otherwise.  Cache the result.
+
+proc check_effective_target_riscv_d { } {
+    return [check_no_compiler_messages riscv_ext_d assembly {
+       #ifndef __riscv_d
+       #error "Not __riscv_d"
+       #endif
+    }]
+}
+
+# Return 1 if the target arch supports the vector extension, 0 otherwise.
+# Cache the result.
+
+proc check_effective_target_riscv_v { } {
+    return [check_no_compiler_messages riscv_ext_v assembly {
+       #ifndef __riscv_v
+       #error "Not __riscv_v"
+       #endif
+    }]
+}
+
+# Return 1 if the target arch supports half float, 0 otherwise.
+# Note, this differs from the test performed by
+# /* dg-skip-if "" { *-*-* } { "*" } { "-march=rv*zfh*" } */
+# in that it takes default behaviour into account.
+# Cache the result.
+
+proc check_effective_target_riscv_zfh { } {
+    return [check_no_compiler_messages riscv_ext_zfh assembly {
+       #ifndef __riscv_zfh
+       #error "Not __riscv_zfh"
+       #endif
+    }]
+}
+
+# Return 1 if we can execute code when using dg-add-options riscv_v
+
+proc check_effective_target_riscv_v_ok { } {
+    # If the target already supports v without any added options,
+    # we may assume we can execute just fine.
+    if { [check_effective_target_riscv_v] } {
+	return 1
+    }
+
+    # check if we can execute vector insns with the given hardware or
+    # simulator
+    set gcc_march [regsub {[[:alnum:]]*} [riscv_get_arch] &v]
+    if { [check_runtime ${gcc_march}_exec {
+	  int main() {  asm("vsetivli t0, 9, e8, m1, tu, ma"); return 0; } } "-march=${gcc_march}"] } {
+	return 1
+    }
+
+    # Possible future extensions: If the target is a simulator, dg-add-options
+    # might change its config to make it allow vector insns, or we might use
+    # options to set special elf flags / sections to effect that.
+
+    return 0
+}
+
+# Return 1 if we can execute code when using dg-add-options riscv_zfh
+
+proc check_effective_target_riscv_zfh_ok { } {
+    # If the target already supports zfh without any added options,
+    # we may assume we can execute just fine.
+    # ??? Other cases we should consider: 
+    # - target / simulator already supports zfh extension - test for that.
+    # - target is a simulator, and dg-add-options knows how to enable zfh support in that simulator
+    if { [check_effective_target_riscv_zfh] } {
+	return 1
+    }
+
+    # check if we can execute zfh insns with the given hardware or
+    # simulator
+    set gcc_march [riscv_get_arch]
+    if { [check_runtime ${gcc_march}_zfh_exec {
+	  int main() {  asm("feq.h a3,fa5,fa4"); return 0; } } "-march=${gcc_march}_zfh"] } {
+	return 1
+    }
+
+    # Possible future extensions: If the target is a simulator, dg-add-options
+    # might change its config to make it allow half float insns, or we might
+    # use options to set special elf flags / sections to effect that.
+
+    return 0
+}
+
+proc riscv_get_arch { } {
+    set gcc_march ""
+    # ??? do we neeed to add more extensions to the list below?
+    foreach ext { i m a f d q c v zicsr zifencei zfh zba zbb zbc zbs } {
+	if { [check_no_compiler_messages  riscv_ext_$ext assembly [string map [list DEF __riscv_$ext] {
+		#ifndef DEF
+		#error "Not DEF"
+		#endif
+    }]] } {
+	    if { [string length $ext] > 1 } {
+		set ext _${ext}
+	    }
+	    set gcc_march $gcc_march$ext
+	}
+	if { [string equal $gcc_march "imafd"] } {
+	    set gcc_march "g"
+	}
+    }
+    if { [check_effective_target_rv32] } {
+	set gcc_march rv32$gcc_march
+    } elseif { [check_effective_target_rv64] } {
+	set gcc_march rv64$gcc_march
+    } else {
+	set gcc_march ""
+    }
+    return "$gcc_march"
+}
+
+proc add_options_for_riscv_d { flags } {
+    if { [lsearch $flags -march=*] >= 0 } {
+	# If there are multiple -march flags, we have to adjust all of them.
+	return [regsub -all -- {((?^|[[:space:]])-march=rv[[:digit:]]*[a-ce-rt-wy]*)d*} $flags \\1d ]
+    }
+    if { [check_effective_target_riscv_d] } {
+	return "$flags"
+    }
+    return "$flags -march=[regsub {[[:alnum:]]*} [riscv_get_arch] &d]"
+}
+
+proc add_options_for_riscv_v { flags } {
+    if { [lsearch $flags -march=*] >= 0 } {
+	# If there are multiple -march flags, we have to adjust all of them.
+	return [regsub -all -- {((?^|[[:space:]])-march=rv[[:digit:]]*[a-rt-uwy]*)v*} $flags \\1v ]
+    }
+    if { [check_effective_target_riscv_v] } {
+	return "$flags"
+    }
+    return "$flags -march=[regsub {[[:alnum:]]*} [riscv_get_arch] &v]"
+}
+
+proc add_options_for_riscv_zfh { flags } {
+    if { [lsearch $flags -march=*] >= 0 } {
+	# If there are multiple -march flags, we have to adjust all of them.
+	set flags [regsub -all -- {(?^|[[:space:]])-march=[[:alnum:]_.]*} $flags &_zfh ]
+	return [regsub -all -- {((?^|[[:space:]])-march=[[:alnum:]_.]*_zfh[[:alnum:]_.]*)_zfh} $flags \\1 ]
+    }
+    if { [check_effective_target_riscv_zfh] } {
+	return "$flags"
+    }
+    return "$flags -march=[riscv_get_arch]_zfh"
+}
+
 # Return 1 if the target OS supports running SSE executables, 0
 # otherwise.  Cache the result.
  

Hi Joern.

> Hmm, you are right.  I personally prefer my version because it allows
> consistent naming of the
> different tests, also easily extendible when new extensions need testing.
> Although the riscv_vector name has the advantage that it is better
> legible for people who are
> not used to dealing with RISC_V extension names.  If we keep
> riscv_vector, it would make
> sense to name the other tests also something more verbose, e.g. change
> riscv_d into
> riscv_double_fp or even riscv_double_precision_floating_point .
> It would be nice to hear other people's opinions on the naming.

I can live with either with a preference for your naming scheme, i.e. 
calling the extensions directly by their name for consistency reasons.
A more verbose scheme might lead to misconceptions later in case we
have several closely related extensions.  There will probably already be
ample discussion during ratification about naming and IMHO we should
not repeat that just to make names more accessible.  If needed we can
still add comments in the respective tests to clarify.
Vector is usually special among architecture extensions but we're not
even consistent with naming in the source itself, so...  

>> Would it make sense to skip the first check here
>> (check_effective_target_riscv_v) so we have a proper runtime check?
> 
> My starting point was that the changing of global testsuite variables around -
> as the original RISC-V vector patches did - is wrong.  The user asked to test
> a particular target (or set targets, for multilibs), and that target
> is the one to test,
> so we can't just assume it has other hardware features that are not implied by
> the target.
> Contrarily, the target that the user requested to test can be assumed to be
> available for testing.  Testing that it actually works is a part of
> the point of the
> test.  If I ask for a dejagnu test for a target that has vector support, I would
> hope that the vector support is also tested, not backing off if it finds that
> there is a problem with the target,
> The way I look at things, when the macro  __riscv_v is defined,
> the compiler asserts that it is compiling for a target that has vector support,
> because it was instructed by configuration / options to emit code for that
> target.  Which we can take as evidence that dejagnu is run with options
> to select that target (either explicitly or by default due to the
> configuration of
> the compiler under test)

Yes, I largely agree with that.  Where I was coming from is that several other
effective target checks will not short circuit the check but always perform it
fully (i.e. interpreting the effective target as the full chain up to execution).
Yet, I can see the appeal of the short circuit as well and in the end it really
doesn't matter all that much.

I would have preferred to replace the existing checks right away in order to
immediately have proper coverage but let's not dwell on that, therefore
LGTM, thanks. 

Regards
 Robin