[Bug target/111828] New: rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[Bug target/111828] New: rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

            Bug ID: 111828
           Summary: rs6000: Parse inline asm string to figure out it
                    requires HTM feature or not.
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: linkw at gcc dot gnu.org
                CC: jan.wassenberg at gmail dot com, john_platts at hotmail dot com,
                    linkw at gcc dot gnu.org, malat at debian dot org
        Depends on: 111366
  Target Milestone: ---
            Target: powerpc64le

Currently as the discussion in PR111366 we simply make the assumption that all
inline asm strings are possible to adopt HTM feature and consider it as
possible as conservative so that set HTM flag.

Technically speaking we are able to parse the inline asm string and figure out
it's HTM related or not.  Excepting for the HTM specific instructions like
.tbegin etc., we also need to take care of those special registers related to
HTM feature. 

+++ This bug was initially created as a clone of Bug #111366 +++


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111366
[Bug 111366] error: inlining failed in call to 'always_inline'
'hwy::PreventElision<int&>(int&)void': target specific option mismatch

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

Peter Bergner <bergner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bergner at gcc dot gnu.org,
                   |                            |dje at gcc dot gnu.org,
                   |                            |meissner at gcc dot gnu.org,
                   |                            |segher at gcc dot gnu.org

--- Comment #1 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #0)
> Technically speaking we are able to parse the inline asm string and figure
> out it's HTM related or not.  Excepting for the HTM specific instructions
> like .tbegin etc., we also need to take care of those special registers
> related to HTM feature. 

I don't like the idea of scanning the inline asm string for HTM instructions. 
The problem is, the user could have used ".long 0x7c00051d" which is a valid
HTM instruction, rather than typing "tbegin.".  There's just no easy way to
handle all possible cases.  It's also a slippery slope of peeking into the
inline asm string and I don't think we want to do that!  If we start doing it
for this, there will just be more and more requests for doing it for other
things.  Let's not.

I also believe that if the user compiles some inline asm using -mcpu=power10,
then the compiler can assume that inline asm only uses features available on
Power10, meaning we can assume the inline asm does not contain HTM or any other
feature not supported on Power10.  If the user compiles a piece of inline asm
that doesn't support the features used in that inline asm, then that is user
error!

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #2 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #1)
> If the user compiles a piece of inline asm that doesn't support the
> features used in that inline asm, then that is user error!

I meant to say: If the user compiles a piece of inline asm using options that
doesn't support the features used in that inline asm, then that is user error!

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #3 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #1)
> (In reply to Kewen Lin from comment #0)
> > Technically speaking we are able to parse the inline asm string and figure
> > out it's HTM related or not.  Excepting for the HTM specific instructions
> > like .tbegin etc., we also need to take care of those special registers
> > related to HTM feature. 
> 
> I don't like the idea of scanning the inline asm string for HTM
> instructions.  The problem is, the user could have used ".long 0x7c00051d"
> which is a valid HTM instruction, rather than typing "tbegin.".  There's
> just no easy way to handle all possible cases.  It's also a slippery slope
> of peeking into the inline asm string and I don't think we want to do that! 
> If we start doing it for this, there will just be more and more requests for
> doing it for other things.  Let's not.

Thanks for the comments, fair enough, I guess that's why we don't have such a
inline asm parser. Another idea is to launch a child process to invoke the
assembler to parse it without htm support (if assembler can support this
fine-grain feature) and see if it works. But it's still complicated.

> 
> I also believe that if the user compiles some inline asm using
> -mcpu=power10, then the compiler can assume that inline asm only uses
> features available on Power10, meaning we can assume the inline asm does not
> contain HTM or any other feature not supported on Power10.  If the user
> compiles a piece of inline asm that doesn't support the features used in
> that inline asm, then that is user error!

We can. But it's not the case that this request aims to solve. 

The motivation of this request is to try our best to make power10 attributed
code inline more power8/power9 attribute code which likely includes some inline
asm but not HTM related as the quoted OSS shows. For now, for one function
which has any non-empty inline asm string, we would consider it's possible to
have HTM code so it's unsafe to inline it.

Users usually think higher cpu attributed code can safely inline lower cpu
attributed code, but it's out of expectation for power10 code inlining
power8/power9 code as we drops HTM from power10. If we can support it better,
users don't need more extra efforts to teach about it.

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[jan.wassenberg at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #4 from Jan Wassenberg <jan.wassenberg at gmail dot com> ---
I understand the slippery slope concern. But the empty asm string is a special
case, we and others use it (with +r output and memory clobber) to prevent
optimizing variables out e.g. during tests.

It seems useful for that to work without running into inlining issues on ppc10
:)

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #5 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Jan Wassenberg from comment #4)
> I understand the slippery slope concern. But the empty asm string is a
> special case, we and others use it (with +r output and memory clobber) to
> prevent optimizing variables out e.g. during tests.

I agree the empty string is a special case and I'm totally fine with the patch
Ke Wen committed/backported to fix your problem.  I'm just against going
further than that and actually trying to parse the contents of the inline asm
string to determine semantically what it contains.

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #6 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #3)
> The motivation of this request is to try our best to make power10 attributed
> code inline more power8/power9 attribute code which likely includes some
> inline asm but not HTM related as the quoted OSS shows. For now, for one
> function which has any non-empty inline asm string, we would consider it's
> possible to have HTM code so it's unsafe to inline it.

We've hit this issue (attempting to inline some Power8/9 function into a
Power10 caller) before with another project (I forget which) and the solution
used was to add no-htm to the attribute target options (ie,
"cpu=power8,no-htm").


> Users usually think higher cpu attributed code can safely inline lower cpu
> attributed code, but it's out of expectation for power10 code inlining
> power8/power9 code as we drops HTM from power10. If we can support it
> better, users don't need more extra efforts to teach about it.

Ideally, we could just go back in time and not enable HTM by default on
Power8/9 and force the user to always use -mhtm if they need HTM support.  That
ship has sailed though.

That said, I think nearly all (all?) HTM usage on Power uses our HTM built-in
functions.  Maybe we could remove OPTION_MASK_HTM from the power8/power9
default flags and only add it back in if we detect the use of an HTM built-in
function?

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #7 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #6)
> That said, I think nearly all (all?) HTM usage on Power uses our HTM
> built-in functions.  Maybe we could remove OPTION_MASK_HTM from the
> power8/power9 default flags and only add it back in if we detect the use of
> an HTM built-in function?

Looking at GLIBC, Power's elision-lock.c use of our htm.h uses inline asm with
HTM instructions and not our HTM built-in functions and it doesn't explicitly
add -mhtm to the command line options like S390 does, so I think this idea
won't work. :-(

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4

--- Comment #8 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #6)
> (In reply to Kewen Lin from comment #3)
> > The motivation of this request is to try our best to make power10 attributed
> > code inline more power8/power9 attribute code which likely includes some
> > inline asm but not HTM related as the quoted OSS shows. For now, for one
> > function which has any non-empty inline asm string, we would consider it's
> > possible to have HTM code so it's unsafe to inline it.
> 
> We've hit this issue (attempting to inline some Power8/9 function into a
> Power10 caller) before with another project (I forget which) and the
> solution used was to add no-htm to the attribute target options (ie,
> "cpu=power8,no-htm").

Yes, I agree that we already have some solutions for that, no-htm option or
no-htm,htm target option attributes. I was thinking unrealistically if we don't
have to teach users about these things (or sometimes users even don't notice
this). But as you pointed out, the inline asm parsing approach looks
impractical.

> 
> > Users usually think higher cpu attributed code can safely inline lower cpu
> > attributed code, but it's out of expectation for power10 code inlining
> > power8/power9 code as we drops HTM from power10. If we can support it
> > better, users don't need more extra efforts to teach about it.
> 
> Ideally, we could just go back in time and not enable HTM by default on
> Power8/9 and force the user to always use -mhtm if they need HTM support. 
> That ship has sailed though.

Florian Weimer raised this point in PR102059, the compatibility was the concern
though, no further comments/discussions on this then. :(

> 
> That said, I think nearly all (all?) HTM usage on Power uses our HTM
> built-in functions.  Maybe we could remove OPTION_MASK_HTM from the
> power8/power9 default flags and only add it back in if we detect the use of
> an HTM built-in function?
>
> Looking at GLIBC, Power's elision-lock.c use of our htm.h uses inline asm
> with HTM instructions and not our HTM built-in functions and it doesn't
> explicitly add -mhtm to the command line options like S390 does, so I think
> this idea won't work. :-(

Thanks for checking!  Sigh...

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #9 from Kewen Lin <linkw at gcc dot gnu.org> ---
Peter had a check on gnu assembler (Thanks!) and found that even with -mpower10
specified it's still able to assemble HTM insns, so it means that for some
callee with power8 attributed has HTM inline asm, it can be successfully
compiled when it's inlined to a caller with power10 attributed (at least gnu
assembler involved). It sounds weird and unexpected that power10 attributed
function calls one power8 attributed function which has HTM insns, but it would
only break during runtime when running on Power10 (weird but safe if no power10
insns are exploited and run on power8 or power9 machine), so the point here is
to avoid inconsistent behaviors before and after inlining.

As Peter suggested, I think it's a good idea to make the inlining aggressive on
inline asm when the involved assembler can support that, but considering that
gcc configuration support --with-as and it's possible to have an assembler
which doesn't support HTM insns at power10 (like in future gnu assembler
changes its behavior if users complains, or other assembler alternatives decide
not to support it to avoid confusion etc.), it seems that we should also check
the given assembler's behavior at configuration?

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-10-26

[Bug target/111828] rs6000: Parse inline asm string to figure out it requires HTM feature or not.

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:b2075291af8810794c7184fd125b991c2341cb1e

commit r14-5142-gb2075291af8810794c7184fd125b991c2341cb1e
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Mon Nov 6 00:14:43 2023 -0600

    rs6000: Consider inline asm as safe if no assembler complains [PR111828]

    As discussed in PR111828, rs6000_update_ipa_fn_target_info
    is much conservative, currently for any non-empty inline
    asm, without any parsing, it would take inline asm could
    have HTM insns.  It means for one function attributed with
    power8 having inline asm, even if it has no HTM insns, we
    don't make a function attributed with power10 inline it.

    Peter pointed out an inline asm parser can be a slippery
    slope, and noticed that the current gnu assembler still
    allows HTM insns even with power10 machine type, so he
    suggested that we can aggressively ignore the handling on
    inline asm, this patch goes for this suggestion.

    Considering that there are a few assembler alternatives
    and assembler can update its behaviors (complaining HTM
    insns at power10 and later cpus sounds reasonable from a
    certain point of view), this patch also checks assembler
    complains on HTM insns at power10 or not.  For a case that
    a caller attributed power10 calls a callee attributed
    power8 having inline asm with HTM insn, without inlining
    at least the compilation succeeds, but if assembler
    complains HTM insns at power10, after inlining the
    compilation would fail.

    The two associated test cases are fine without and with
    this patch (effective target takes effect or not).

            PR target/111828

    gcc/ChangeLog:

            * config.in: Regenerate.
            * config/rs6000/rs6000.cc (rs6000_update_ipa_fn_target_info): Guard
            inline asm handling under !HAVE_AS_POWER10_HTM.
            * configure: Regenerate.
            * configure.ac: Detect assembler support for HTM insns at power10.

    gcc/testsuite/ChangeLog:

            * lib/target-supports.exp
            (check_effective_target_powerpc_as_p10_htm): New proc.
            * g++.target/powerpc/pr111828-1.C: New test.
            * g++.target/powerpc/pr111828-2.C: New test.

[Bug target/111828] rs6000: Aggressive inline callee with inline asm without complains from assemblers

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:b20e79659b08f02614d83c4d7dd083411c96dbd9

commit r12-9980-gb20e79659b08f02614d83c4d7dd083411c96dbd9
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Mon Nov 6 00:14:43 2023 -0600

    rs6000: Consider inline asm as safe if no assembler complains [PR111828]

    As discussed in PR111828, rs6000_update_ipa_fn_target_info
    is much conservative, currently for any non-empty inline
    asm, without any parsing, it would take inline asm could
    have HTM insns.  It means for one function attributed with
    power8 having inline asm, even if it has no HTM insns, we
    don't make a function attributed with power10 inline it.

    Peter pointed out an inline asm parser can be a slippery
    slope, and noticed that the current gnu assembler still
    allows HTM insns even with power10 machine type, so he
    suggested that we can aggressively ignore the handling on
    inline asm, this patch goes for this suggestion.

    Considering that there are a few assembler alternatives
    and assembler can update its behaviors (complaining HTM
    insns at power10 and later cpus sounds reasonable from a
    certain point of view), this patch also checks assembler
    complains on HTM insns at power10 or not.  For a case that
    a caller attributed power10 calls a callee attributed
    power8 having inline asm with HTM insn, without inlining
    at least the compilation succeeds, but if assembler
    complains HTM insns at power10, after inlining the
    compilation would fail.

    The two associated test cases are fine without and with
    this patch (effective target takes effect or not).

            PR target/111828

    gcc/ChangeLog:

            * config.in: Regenerate.
            * config/rs6000/rs6000.cc (rs6000_update_ipa_fn_target_info): Guard
            inline asm handling under !HAVE_AS_POWER10_HTM.
            * configure: Regenerate.
            * configure.ac: Detect assembler support for HTM insns at power10.

    gcc/testsuite/ChangeLog:

            * lib/target-supports.exp
            (check_effective_target_powerpc_as_p10_htm): New proc.
            * g++.target/powerpc/pr111828-1.C: New test.
            * g++.target/powerpc/pr111828-2.C: New test.

    (cherry picked from commit b2075291af8810794c7184fd125b991c2341cb1e)

[Bug target/111828] rs6000: Aggressive inline callee with inline asm without complains from assemblers

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:63690ce8174c8b8d13e8d1a1f01b19e4f6d93e9e

commit r13-8070-g63690ce8174c8b8d13e8d1a1f01b19e4f6d93e9e
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Mon Nov 6 00:14:43 2023 -0600

    rs6000: Consider inline asm as safe if no assembler complains [PR111828]

    As discussed in PR111828, rs6000_update_ipa_fn_target_info
    is much conservative, currently for any non-empty inline
    asm, without any parsing, it would take inline asm could
    have HTM insns.  It means for one function attributed with
    power8 having inline asm, even if it has no HTM insns, we
    don't make a function attributed with power10 inline it.

    Peter pointed out an inline asm parser can be a slippery
    slope, and noticed that the current gnu assembler still
    allows HTM insns even with power10 machine type, so he
    suggested that we can aggressively ignore the handling on
    inline asm, this patch goes for this suggestion.

    Considering that there are a few assembler alternatives
    and assembler can update its behaviors (complaining HTM
    insns at power10 and later cpus sounds reasonable from a
    certain point of view), this patch also checks assembler
    complains on HTM insns at power10 or not.  For a case that
    a caller attributed power10 calls a callee attributed
    power8 having inline asm with HTM insn, without inlining
    at least the compilation succeeds, but if assembler
    complains HTM insns at power10, after inlining the
    compilation would fail.

    The two associated test cases are fine without and with
    this patch (effective target takes effect or not).

            PR target/111828

    gcc/ChangeLog:

            * config.in: Regenerate.
            * config/rs6000/rs6000.cc (rs6000_update_ipa_fn_target_info): Guard
            inline asm handling under !HAVE_AS_POWER10_HTM.
            * configure: Regenerate.
            * configure.ac: Detect assembler support for HTM insns at power10.

    gcc/testsuite/ChangeLog:

            * lib/target-supports.exp
            (check_effective_target_powerpc_as_p10_htm): New proc.
            * g++.target/powerpc/pr111828-1.C: New test.
            * g++.target/powerpc/pr111828-2.C: New test.

    (cherry picked from commit b2075291af8810794c7184fd125b991c2341cb1e)

[Bug target/111828] rs6000: Aggressive inline callee with inline asm without complains from assemblers

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111828

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #13 from Kewen Lin <linkw at gcc dot gnu.org> ---
The enhancement should work on trunk, gcc-13 and gcc-12 now.