Current semantic is a little strange when multiple --group options are
specified.
In this case,  only instructions in *all* these groups (i.e., intersection)
are used for
generation, which is not very useful at all.  This patch changes the
semantic to
include all instructions in these groups (i.e., union) for sequence
generation.

Signed-off-by: Jun Sun <jsun@junsun.net>
---
 risugen                | 4 ++--
 risugen_arm.pm         | 1 +
 risugen_loongarch64.pm | 1 +
 risugen_m68k.pm        | 1 +
 risugen_ppc64.pm       | 1 +
 5 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/risugen b/risugen
index 360112f..f88c22a 100755
--- a/risugen
+++ b/risugen
@@ -264,7 +264,7 @@ sub select_insn_keys ()
     if (@groups) {
         @insn_keys = grep {
             defined($insn_details{$_}->{groups}) &&
-                scalar @groups ==
get_intersection([$insn_details{$_}->{groups}, \@groups])
+                0 != scalar(get_intersection([$insn_details{$_}->{groups},
\@groups]))
         } @insn_keys
     }
     # Get a list of the insn keys which are permitted by the re patterns
@@ -297,7 +297,7 @@ Valid options:
     --fpscr n    : set initial FPSCR (arm) or FPCR (aarch64) value
(default is 0)
     --condprob p : [ARM only] make instructions conditional with
probability p
                    (default is 0, ie all instructions are always executed)
-    --group name[,name..]: only use instructions in all defined groups
+    --group name[,name..]: only use instructions in the specified groups
     --pattern re[,re...] : only use instructions matching regular
expression
                    Each re must match a full word (that is, we match on
                    the perl regex '\\b((re)|(re))\\b'). This means that
diff --git a/risugen_arm.pm b/risugen_arm.pm
index 2dc144d..dc08ec0 100644
--- a/risugen_arm.pm
+++ b/risugen_arm.pm
@@ -1112,6 +1112,7 @@ sub write_test_code($$$$$$$$$)
     }

     print "Generating code using patterns: @keys...\n";
+    print "Total insn patterns : " . $#keys . "\n";
     progress_start(78, $numinsns);

     if ($fp_enabled) {
diff --git a/risugen_loongarch64.pm b/risugen_loongarch64.pm
index 3b1b4f9..f2a6fe7 100644
--- a/risugen_loongarch64.pm
+++ b/risugen_loongarch64.pm
@@ -482,6 +482,7 @@ sub write_test_code($)
     }

     print "Generating code using patterns: @keys...\n";
+    print "Total insn patterns : " . $#keys . "\n";
     progress_start(78, $numinsns);

     if ($fp_enabled) {
diff --git a/risugen_m68k.pm b/risugen_m68k.pm
index 85fc3da..76af84b 100644
--- a/risugen_m68k.pm
+++ b/risugen_m68k.pm
@@ -181,6 +181,7 @@ sub write_test_code($)
     }

     print "Generating code using patterns: @keys...\n";
+    print "Total insn patterns : " . $#keys . "\n";
     progress_start(78, $numinsns);

     if (grep { defined($insn_details{$_}->{blocks}->{"memory"}) } @keys) {
diff --git a/risugen_ppc64.pm b/risugen_ppc64.pm
index 4bc2d62..e6d0456 100644
--- a/risugen_ppc64.pm
+++ b/risugen_ppc64.pm
@@ -392,6 +392,7 @@ sub write_test_code($)
     }

     print "Generating code using patterns: @keys...\n";
+    print "Total insn patterns : " . $#keys . "\n";
     progress_start(78, $numinsns);

     if (grep { defined($insn_details{$_}->{blocks}->{"memory"}) } @keys) {
-- 
2.34.1

On Wed, 3 May 2023 at 17:35, Jun Sun <jsun@junsun.net> wrote:
>
>
> Current semantic is a little strange when multiple --group options are specified.
> In this case,  only instructions in *all* these groups (i.e., intersection) are used for
> generation, which is not very useful at all.  This patch changes the semantic to
> include all instructions in these groups (i.e., union) for sequence generation.

The commit message which added the --group option specifically
documents a case where it is useful:

      ./risugen --group v8.2,Cryptographic aarch64.risu v8.2-crypto.bin

where you want to say "only test the v8.2 crypto insns"
(i.e. not any crypto insns from other architecture versions,
and not any non-crypto insns).
Changing the semantics to union would break this.

Being able to specify that you want insns from multiple
groups seems like it would be useful, but we should add
it in a way that doesn't break the existing uses.

One idea that occurs to me is that you could allow
multiple --group options to mean "union of these"
and multiple groups within a --group to mean "intersection".
So for instance
 --group v8.2,Cryptographic --group v8_3_compnum
would select all the insns that are
   (v8.2 AND cryptographic) OR v8_3_compnum

(This does technically break some existing commandlines
because the current code makes "--group A --group B"
do the same thing as "--group A,B".)

Alex, you added the --group option -- what do you think?

> diff --git a/risugen_arm.pm b/risugen_arm.pm
> index 2dc144d..dc08ec0 100644
> --- a/risugen_arm.pm
> +++ b/risugen_arm.pm
> @@ -1112,6 +1112,7 @@ sub write_test_code($$$$$$$$$)
>      }
>
>      print "Generating code using patterns: @keys...\n";
> +    print "Total insn patterns : " . $#keys . "\n";
>      progress_start(78, $numinsns);
>
>      if ($fp_enabled) {

These changes seem unrelated to the --group option.

thanks
-- PMM

I see. I think your suggestion makes sense, i.e., groups specified in one
line means intersection of groups, while multiple --group lines mean union
of groups.

BTW, I have moved along and added many new features to risu, including
generating instructions as shared libraries (instead of raw binary files),
multiple instruction sequence, etc.  If these are interesting to the
community, I will try to find time and submit.  There are a lot of works to
make a proper patch than just making my hack. :)

Jun

On Tue, May 9, 2023 at 5:43 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Wed, 3 May 2023 at 17:35, Jun Sun <jsun@junsun.net> wrote:
> >
> >
> > Current semantic is a little strange when multiple --group options are
> specified.
> > In this case,  only instructions in *all* these groups (i.e.,
> intersection) are used for
> > generation, which is not very useful at all.  This patch changes the
> semantic to
> > include all instructions in these groups (i.e., union) for sequence
> generation.
>
> The commit message which added the --group option specifically
> documents a case where it is useful:
>
>       ./risugen --group v8.2,Cryptographic aarch64.risu v8.2-crypto.bin
>
> where you want to say "only test the v8.2 crypto insns"
> (i.e. not any crypto insns from other architecture versions,
> and not any non-crypto insns).
> Changing the semantics to union would break this.
>
> Being able to specify that you want insns from multiple
> groups seems like it would be useful, but we should add
> it in a way that doesn't break the existing uses.
>
> One idea that occurs to me is that you could allow
> multiple --group options to mean "union of these"
> and multiple groups within a --group to mean "intersection".
> So for instance
>  --group v8.2,Cryptographic --group v8_3_compnum
> would select all the insns that are
>    (v8.2 AND cryptographic) OR v8_3_compnum
>
> (This does technically break some existing commandlines
> because the current code makes "--group A --group B"
> do the same thing as "--group A,B".)
>
> Alex, you added the --group option -- what do you think?
>
> > diff --git a/risugen_arm.pm b/risugen_arm.pm
> > index 2dc144d..dc08ec0 100644
> > --- a/risugen_arm.pm
> > +++ b/risugen_arm.pm
> > @@ -1112,6 +1112,7 @@ sub write_test_code($$$$$$$$$)
> >      }
> >
> >      print "Generating code using patterns: @keys...\n";
> > +    print "Total insn patterns : " . $#keys . "\n";
> >      progress_start(78, $numinsns);
> >
> >      if ($fp_enabled) {
>
> These changes seem unrelated to the --group option.
>
> thanks
> -- PMM
>