On Wed, 3 Jul 2024, Jakub Jelinek wrote:
> On Tue, Jun 11, 2024 at 01:20:50PM +0200, Richard Biener wrote:
> > When the operand is gimplified to an extract of a register or a
> > register we have to disallow memory as we otherwise fail to
> > gimplify it properly. Instead of
> >
> > __asm__("" : "=rm" __imag <r>);
> >
> > we want
> >
> > __asm__("" : "=rm" D.2772);
> > _1 = REALPART_EXPR <r>;
> > r = COMPLEX_EXPR <_1, D.2772>;
> >
> > otherwise SSA rewrite will fail and generate wrong code with 'r'
> > left bare in the asm output.
> >
> > Bootstrap and regtest in progress on x86_64-unknown-linux-gnu.
> >
> > I've made the testcase hopefully generic enough (the bug used =X
> > which I'm not sure is portable - I've used _Complex int so 'r'
> > has a chance to work).
>
> > --- a/gcc/gimplify.cc
> > +++ b/gcc/gimplify.cc
> > @@ -7044,6 +7044,22 @@ gimplify_asm_expr (tree *expr_p, gimple_seq *pre_p,
> > gimple_seq *post_p)
> > ret = tret;
> > }
> >
> > + /* If the gimplified operand is a register we do not allow memory.
> > */
> > + if (allows_mem
> > + && (is_gimple_reg (TREE_VALUE (link))
> > + || (handled_component_p (TREE_VALUE (link))
> > + && is_gimple_reg (TREE_OPERAND (TREE_VALUE (link), 0)))))
> > + {
> > + if (allows_reg)
> > + allows_mem = 0;
> > + else
> > + {
> > + error ("impossible constraint in %<asm%>");
> > + error ("non-memory output %d must stay in memory", i);
> > + return GS_ERROR;
>
> Does this else part ever trigger or could it be just gcc_assert (allows_reg)?
I've added this just for completeness, so I think I can make it an
assert if we consider this invalid GENERIC.
> E.g. C FE build_asm_expr has
> /* If the operand is going to end up in memory,
> mark it addressable. */
> if (!allows_reg && !c_mark_addressable (output))
> Or C++ FE finish_asm_stmt:
> /* If the operand is going to end up in memory,
> mark it addressable. */
> if (!allows_reg && !cxx_mark_addressable (*op))
>
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr115426.c
> > @@ -0,0 +1,9 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-std=gnu11" } */
> > +
> > +_Complex int fcs()
> > +{
> > + _Complex int r;
>
> It would be useful to initialize r or at least __real__ r
> before the asm as we return it whole and I think the bug
> should trigger with that too.
The bug triggers also for
_Complex int fcs (_Complex int r)
{
__asm__("" : "=rm" (__imag__ r));
return r;
}
> > + __asm__("" : "=rm" (__imag__ r));
> > + return r;
> > +}
>
> Also, it would be nice to cover also the "=m" case in another
> function to make sure that still works.
Done.
I'm re-testing the following.
Richard.
>From 40023cac83562a1451aba550533d042fec1c144e Mon Sep 17 00:00:00 2001
From: Richard Biener <rguent...@suse.de>
Date: Tue, 11 Jun 2024 13:11:08 +0200
Subject: [PATCH] middle-end/115426 - wrong gimplification of "rm" asm output
operand
To: gcc-patches@gcc.gnu.org
When the operand is gimplified to an extract of a register or a
register we have to disallow memory as we otherwise fail to
gimplify it properly. Instead of
__asm__("" : "=rm" __imag <r>);
we want
__asm__("" : "=rm" D.2772);
_1 = REALPART_EXPR <r>;
r = COMPLEX_EXPR <_1, D.2772>;
otherwise SSA rewrite will fail and generate wrong code with 'r'
left bare in the asm output.
PR middle-end/115426
* gimplify.cc (gimplify_asm_expr): Handle "rm" output
constraint gimplified to a register (operation).
* gcc.dg/pr115426.c: New testcase.
---
gcc/gimplify.cc | 10 ++++++++++
gcc/testsuite/gcc.dg/pr115426.c | 14 ++++++++++++++
2 files changed, 24 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/pr115426.c
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 04875e8df29..13189ddee99 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -7044,6 +7044,16 @@ gimplify_asm_expr (tree *expr_p, gimple_seq *pre_p,
gimple_seq *post_p)
ret = tret;
}
+ /* If the gimplified operand is a register we do not allow memory. */
+ if (allows_mem
+ && (is_gimple_reg (TREE_VALUE (link))
+ || (handled_component_p (TREE_VALUE (link))
+ && is_gimple_reg (TREE_OPERAND (TREE_VALUE (link), 0)))))
+ {
+ gcc_assert (allows_reg);
+ allows_mem = 0;
+ }
+
/* If the constraint does not allow memory make sure we gimplify
it to a register if it is not already but its base is. This
happens for complex and vector components. */
diff --git a/gcc/testsuite/gcc.dg/pr115426.c b/gcc/testsuite/gcc.dg/pr115426.c
new file mode 100644
index 00000000000..02bfc3f21fa
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr115426.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-std=gnu11" } */
+
+_Complex int fcs (_Complex int r)
+{
+ __asm__("" : "=rm" (__imag__ r));
+ return r;
+}
+
+_Complex int fcs2 (_Complex int r)
+{
+ __asm__("" : "=m" (__imag__ r));
+ return r;
+}
--
2.35.3
