Re: [PATCH] Do not fold addressable operands of "m" into non-addressable (PR inline-asm/50571)

[Richard Guenther <richard dot guenther at gmail dot com>]

On Fri, Sep 30, 2011 at 8:17 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> GCC on the following testcase warns
> warning: use of memory input without lvalue in asm operand 0 is deprecated [enabled by default]
> starting with 4.6, but the source actually had an lvalue there (I don't
> think we should forbid for input operands const qualified memory).
> On "m" (1) in the source we would error out early, but here we
> fold that "m" (*(int *) var) during gimple folding into
> "m" (1). ?The following patch makes sure that we don't fold an lvalue
> into non-lvalue for asm operands that allow memory, but not reg.
>
> This has been originally reported on Linux kernel bitops, which IMHO
> are invalid too, they say to the compiler that "m" (*(unsigned long *)array)
> is being read, but in fact the inline asm relies on the whole array to
> be after that too. ?IMHO one should tell the compiler about that,
> through "m" (*(struct { unsigned long __l[0x10000000]; } *)array)
> or similar.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.6?

Hmm, I don't think this change is ok.  We rely on maybe_fold_reference
to re-fold mem-refs to valid gimple form (from propagating say
&a.b.c to MEM[p, 4] which first gives the invalid MEM[&a.b.c, 4] and
then the folding changes this to MEM[&a, 12]).  You need to preserve that
and only disable constant folding.

Thus I suggest to add a parameter to maybe_fold_reference that says
whether to try constant folding (we already have is_lhs, so why not
use that?)

Thanks,
Richard.

> 2011-09-30 ?Jakub Jelinek ?<jakub@redhat.com>
>
> ? ? ? ?PR inline-asm/50571
> ? ? ? ?* gimple-fold.c (fold_stmt_1) <case GIMPLE_ASM>: If
> ? ? ? ?constraints allow mem and not reg, don't optimize lvalues
> ? ? ? ?into non-lvalues.
>
> ? ? ? ?* gcc.dg/pr50571.c: New test.
>
> --- gcc/gimple-fold.c.jj ? ? ? ?2011-09-29 14:25:46.000000000 +0200
> +++ gcc/gimple-fold.c ? 2011-09-29 21:25:00.000000000 +0200
> @@ -1201,28 +1201,55 @@ fold_stmt_1 (gimple_stmt_iterator *gsi,
>
> ? ? case GIMPLE_ASM:
> ? ? ? /* Fold *& in asm operands. ?*/
> - ? ? ?for (i = 0; i < gimple_asm_noutputs (stmt); ++i)
> - ? ? ? {
> - ? ? ? ? tree link = gimple_asm_output_op (stmt, i);
> - ? ? ? ? tree op = TREE_VALUE (link);
> - ? ? ? ? if (REFERENCE_CLASS_P (op)
> - ? ? ? ? ? ? && (op = maybe_fold_reference (op, true)) != NULL_TREE)
> - ? ? ? ? ? {
> - ? ? ? ? ? ? TREE_VALUE (link) = op;
> - ? ? ? ? ? ? changed = true;
> - ? ? ? ? ? }
> - ? ? ? }
> - ? ? ?for (i = 0; i < gimple_asm_ninputs (stmt); ++i)
> - ? ? ? {
> - ? ? ? ? tree link = gimple_asm_input_op (stmt, i);
> - ? ? ? ? tree op = TREE_VALUE (link);
> - ? ? ? ? if (REFERENCE_CLASS_P (op)
> - ? ? ? ? ? ? && (op = maybe_fold_reference (op, false)) != NULL_TREE)
> - ? ? ? ? ? {
> - ? ? ? ? ? ? TREE_VALUE (link) = op;
> - ? ? ? ? ? ? changed = true;
> - ? ? ? ? ? }
> - ? ? ? }
> + ? ? ?{
> + ? ? ? size_t noutputs;
> + ? ? ? const char **oconstraints;
> + ? ? ? const char *constraint;
> + ? ? ? bool allows_mem, allows_reg, is_inout;
> +
> + ? ? ? noutputs = gimple_asm_noutputs (stmt);
> + ? ? ? oconstraints = XALLOCAVEC (const char *, noutputs);
> +
> + ? ? ? for (i = 0; i < gimple_asm_noutputs (stmt); ++i)
> + ? ? ? ? {
> + ? ? ? ? ? tree link = gimple_asm_output_op (stmt, i);
> + ? ? ? ? ? tree op = TREE_VALUE (link);
> + ? ? ? ? ? constraint
> + ? ? ? ? ? ? = TREE_STRING_POINTER (TREE_VALUE (TREE_PURPOSE (link)));
> + ? ? ? ? ? oconstraints[i] = constraint;
> + ? ? ? ? ? parse_output_constraint (&constraint, i, 0, 0, &allows_mem,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&allows_reg, &is_inout);
> + ? ? ? ? ? if (REFERENCE_CLASS_P (op)
> + ? ? ? ? ? ? ? && (op = maybe_fold_reference (op, true)) != NULL_TREE
> + ? ? ? ? ? ? ? && (allows_reg
> + ? ? ? ? ? ? ? ? ? || !allows_mem
> + ? ? ? ? ? ? ? ? ? || is_gimple_lvalue (op)
> + ? ? ? ? ? ? ? ? ? || !is_gimple_lvalue (TREE_VALUE (link))))
> + ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? TREE_VALUE (link) = op;
> + ? ? ? ? ? ? ? changed = true;
> + ? ? ? ? ? ? }
> + ? ? ? ? }
> + ? ? ? for (i = 0; i < gimple_asm_ninputs (stmt); ++i)
> + ? ? ? ? {
> + ? ? ? ? ? tree link = gimple_asm_input_op (stmt, i);
> + ? ? ? ? ? tree op = TREE_VALUE (link);
> + ? ? ? ? ? constraint
> + ? ? ? ? ? ? = TREE_STRING_POINTER (TREE_VALUE (TREE_PURPOSE (link)));
> + ? ? ? ? ? parse_input_constraint (&constraint, 0, 0, noutputs, 0,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? oconstraints, &allows_mem, &allows_reg);
> + ? ? ? ? ? if (REFERENCE_CLASS_P (op)
> + ? ? ? ? ? ? ? && (op = maybe_fold_reference (op, false)) != NULL_TREE
> + ? ? ? ? ? ? ? && (allows_reg
> + ? ? ? ? ? ? ? ? ? || !allows_mem
> + ? ? ? ? ? ? ? ? ? || is_gimple_lvalue (op)
> + ? ? ? ? ? ? ? ? ? || !is_gimple_lvalue (TREE_VALUE (link))))
> + ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? TREE_VALUE (link) = op;
> + ? ? ? ? ? ? ? changed = true;
> + ? ? ? ? ? ? }
> + ? ? ? ? }
> + ? ? ?}
> ? ? ? break;
>
> ? ? case GIMPLE_DEBUG:
> --- gcc/testsuite/gcc.dg/pr50571.c.jj ? 2011-09-29 21:28:05.000000000 +0200
> +++ gcc/testsuite/gcc.dg/pr50571.c ? ? ?2011-09-29 21:30:08.000000000 +0200
> @@ -0,0 +1,11 @@
> +/* PR inline-asm/50571 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +static const int var[4] = { 1, 2, 3, 4 };
> +
> +void
> +foo (void)
> +{
> + ?__asm volatile ("" : : "m" (*(int *) var));
> +}
>
> ? ? ? ?Jakub
>