Re: RISCV test infrastructure for d / v / zfh extensions

[Joern Rennecke <joern.rennecke@embecosm.com>]

[-- Attachment #1: Type: text/plain, Size: 6408 bytes --]

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.

[-- Attachment #2: t-supp-diff-20230815.txt --]
[-- Type: text/plain, Size: 6344 bytes --]

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.