On Tue, 1 Aug 2023 at 14:44, Robin Dapp 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.