[PATCH 0/2] meson: Try to clarify TCG / TCI options for new users

[PATCH 0/2] meson: Try to clarify TCG / TCI options for new users

[Philippe Mathieu-Daudé]

Some new users get confused between 'TCG' and 'TCI' and enable
TCI when TCG is better for they needs. Try to clarify it is
better to not use TCI when native backend is available.

Note, before Meson, warnings were summarized at the end of
./configure. Now they are displayed earlier, and likely
missed IMHO. No clue how to improve that :/

Based-on: <20210121095616.1471869-1-philmd@redhat.com>

Philippe Mathieu-Daudé (2):
  meson: Explicit TCG backend used
  meson: Warn when TCI is selected but TCG backend is available

 meson.build | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.26.2

[PATCH 1/2] meson: Explicit TCG backend used

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 meson.build | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 8535a83fb70..0a645e54662 100644
--- a/meson.build
+++ b/meson.build
@@ -2419,8 +2419,12 @@
 endif
 summary_info += {'TCG support':       config_all.has_key('CONFIG_TCG')}
 if config_all.has_key('CONFIG_TCG')
+  if config_host.has_key('CONFIG_TCG_INTERPRETER')
+    summary_info += {'TCG backend':   'TCG interpreter (experimental)'}
+  else
+    summary_info += {'TCG backend':   'native (@0@)'.format(cpu)}
+  endif
   summary_info += {'TCG debug enabled': config_host.has_key('CONFIG_DEBUG_TCG')}
-  summary_info += {'TCG interpreter':   config_host.has_key('CONFIG_TCG_INTERPRETER')}
 endif
 summary_info += {'target list':       ' '.join(target_dirs)}
 if have_system
-- 
2.26.2

[PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available

[Philippe Mathieu-Daudé]

Some new users get confused with 'TCG' and 'TCI', and enable TCI
support expecting to enable TCG.

Emit a warning when native TCG backend is available on the
host architecture, mentioning this is a suboptimal configuration.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 meson.build | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/meson.build b/meson.build
index 0a645e54662..7aa52d306c6 100644
--- a/meson.build
+++ b/meson.build
@@ -234,6 +234,9 @@
       error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
     endif
   endif
+  if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
+    warning('Experimental TCI requested while native TCG is available on @0@, suboptimal performance expected'.format(cpu))
+  endif
   accelerators += 'CONFIG_TCG'
   config_host += { 'CONFIG_TCG': 'y' }
 endif
-- 
2.26.2

Re: [PATCH 1/2] meson: Explicit TCG backend used

[Thomas Huth]

On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   meson.build | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 8535a83fb70..0a645e54662 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2419,8 +2419,12 @@
>   endif
>   summary_info += {'TCG support':       config_all.has_key('CONFIG_TCG')}
>   if config_all.has_key('CONFIG_TCG')
> +  if config_host.has_key('CONFIG_TCG_INTERPRETER')
> +    summary_info += {'TCG backend':   'TCG interpreter (experimental)'}

maybe say "experimental and slow" in the parentheses?

> +  else
> +    summary_info += {'TCG backend':   'native (@0@)'.format(cpu)}
> +  endif
>     summary_info += {'TCG debug enabled': config_host.has_key('CONFIG_DEBUG_TCG')}
> -  summary_info += {'TCG interpreter':   config_host.has_key('CONFIG_TCG_INTERPRETER')}
>   endif
>   summary_info += {'target list':       ' '.join(target_dirs)}
>   if have_system
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available

[Thomas Huth]

On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
> Some new users get confused with 'TCG' and 'TCI', and enable TCI
> support expecting to enable TCG.
> 
> Emit a warning when native TCG backend is available on the
> host architecture, mentioning this is a suboptimal configuration.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   meson.build | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 0a645e54662..7aa52d306c6 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -234,6 +234,9 @@
>         error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
>       endif
>     endif
> +  if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
> +    warning('Experimental TCI requested while native TCG is available on @0@, suboptimal performance expected'.format(cpu))
> +  endif
>     accelerators += 'CONFIG_TCG'
>     config_host += { 'CONFIG_TCG': 'y' }
>   endif
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

... maybe you could also add some wording to the help text of the configure 
script? Or maybe we could simply rename the "--enable-tcg-interpreter" 
option into "--enable-tci" to avoid confusion with the normal TCG ?

Re: [PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available

[Philippe Mathieu-Daudé]

On Fri, Jan 22, 2021 at 10:43 AM Thomas Huth <thuth@redhat.com> wrote:
> On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
> > Some new users get confused with 'TCG' and 'TCI', and enable TCI
> > support expecting to enable TCG.
> >
> > Emit a warning when native TCG backend is available on the
> > host architecture, mentioning this is a suboptimal configuration.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >   meson.build | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/meson.build b/meson.build
> > index 0a645e54662..7aa52d306c6 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -234,6 +234,9 @@
> >         error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
> >       endif
> >     endif
> > +  if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
> > +    warning('Experimental TCI requested while native TCG is available on @0@, suboptimal performance expected'.format(cpu))
> > +  endif
> >     accelerators += 'CONFIG_TCG'
> >     config_host += { 'CONFIG_TCG': 'y' }
> >   endif
> >
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
> ... maybe you could also add some wording to the help text of the configure
> script? Or maybe we could simply rename the "--enable-tcg-interpreter"
> option into "--enable-tci" to avoid confusion with the normal TCG ?

I also thought about renaming as --enable-experimental-tci but then doubt that
would help, some users just want to enable everything :)

Re: [PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available

[Thomas Huth]

On 22/01/2021 10.46, Philippe Mathieu-Daudé wrote:
> On Fri, Jan 22, 2021 at 10:43 AM Thomas Huth <thuth@redhat.com> wrote:
>> On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
>>> Some new users get confused with 'TCG' and 'TCI', and enable TCI
>>> support expecting to enable TCG.
>>>
>>> Emit a warning when native TCG backend is available on the
>>> host architecture, mentioning this is a suboptimal configuration.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>    meson.build | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 0a645e54662..7aa52d306c6 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -234,6 +234,9 @@
>>>          error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
>>>        endif
>>>      endif
>>> +  if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
>>> +    warning('Experimental TCI requested while native TCG is available on @0@, suboptimal performance expected'.format(cpu))
>>> +  endif
>>>      accelerators += 'CONFIG_TCG'
>>>      config_host += { 'CONFIG_TCG': 'y' }
>>>    endif
>>>
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>> ... maybe you could also add some wording to the help text of the configure
>> script? Or maybe we could simply rename the "--enable-tcg-interpreter"
>> option into "--enable-tci" to avoid confusion with the normal TCG ?
> 
> I also thought about renaming as --enable-experimental-tci but then doubt that
> would help, some users just want to enable everything :)

Then we should rename it into --disable-native-tcg ;-)

  Thomas

Re: [PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available

[Stefan Weil]

Am 22.01.21 um 10:47 schrieb Thomas Huth:

> On 22/01/2021 10.46, Philippe Mathieu-Daudé wrote:
>> On Fri, Jan 22, 2021 at 10:43 AM Thomas Huth <thuth@redhat.com> wrote:
>>> On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
>>>> Some new users get confused with 'TCG' and 'TCI', and enable TCI
>>>> support expecting to enable TCG.
>>>>
>>>> Emit a warning when native TCG backend is available on the
>>>> host architecture, mentioning this is a suboptimal configuration.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>    meson.build | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/meson.build b/meson.build
>>>> index 0a645e54662..7aa52d306c6 100644
>>>> --- a/meson.build
>>>> +++ b/meson.build
>>>> @@ -234,6 +234,9 @@
>>>>          error('Unsupported CPU @0@, try 
>>>> --enable-tcg-interpreter'.format(cpu))
>>>>        endif
>>>>      endif
>>>> +  if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in 
>>>> supported_cpus
>>>> +    warning('Experimental TCI requested while native TCG is 
>>>> available on @0@, suboptimal performance expected'.format(cpu))
>>>> +  endif
>>>>      accelerators += 'CONFIG_TCG'
>>>>      config_host += { 'CONFIG_TCG': 'y' }
>>>>    endif
>>>>
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>
>>> ... maybe you could also add some wording to the help text of the 
>>> configure
>>> script? Or maybe we could simply rename the "--enable-tcg-interpreter"
>>> option into "--enable-tci" to avoid confusion with the normal TCG ?
>>
>> I also thought about renaming as --enable-experimental-tci but then 
>> doubt that
>> would help, some users just want to enable everything :)
>
> Then we should rename it into --disable-native-tcg ;-)
>
>  Thomas
>

Both patches are fine (also optionally with the suggested addition of 
"slow"), so

Reviewed-by: Stefan Weil <sw@weilnetz.de>

I think that --enable-tci would increase the TCI/TCG confusion and 
suggest to keep the current --enable-tcg-interpreter as most experts 
know that an interpreter is usually something which is slow.

As soon as time permits I also plan to make a new effort to integrate 
TCI as a run time option. Some years ago it was not possible to have 
code which supports both native and interpreted TCG, but this might have 
changed now. If it is possible, this would simplify CI a lot, and users 
could select the interpreter via a command line argument.

Stefan