Missing DSE opportunity when second store is done in inline assembly

[Quuxplusone]

Bugzilla Link	PR44913
Status	NEW
Importance	P enhancement
Reported by	Alexander Potapenko (glider@google.com)
Reported on	2020-02-14 08:12:57 -0800
Last modified on	2020-02-19 18:52:59 -0800
Version	trunk
Hardware	PC Linux
CC	dancol@google.com, echristo@gmail.com, efriedma@quicinc.com, florian_hahn@apple.com, jyknight@google.com, llvm-bugs@lists.llvm.org, llvm-bugzilla@jfbastien.com, ndesaulniers@google.com, srhines@google.com, vitalybuka@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

As reported by Daniel Colascione, Clang fails to remove the dead store in the
following program:

 extern int foo(int* bar);

 int bar()
 {
    int x = 5;
    asm("nop\n\tnop\n\tnop" : "=m" (x));
    return foo(&x);
 }

see https://gcc.godbolt.org/z/c2fMNH (bad, clang) vs.
https://gcc.godbolt.org/z/a_biz5 (good, GCC).

This probably prevents it from deleting redundant initialization before
copy_from_user() in the Linux kernel.

[Quuxplusone]

We currently don't have any IR attribute to express that a call always overwrites some set number of bytes. We probably need to add something like that to describe the semantics, and teach DSE to handle it. Currently, we only have writeonly, which isn't enough.

In terms of actually figuring out when it's appropriate to apply that attribute, I don't want to be the one to figure out the correct logic. (What's the effect of a "memory" clobber ? "asm volatile"? "+m"?)

[Quuxplusone]

You have to be careful because the IR attribute can't be inferred by the optimizer, unless the function is local to the TU. It can be 100% trusted when the frontend applies the attribute. That's because ODR (IIRC Vedant had a post describing why that was the case).

For details of how I think this should be done, see this post: lists.llvm.org/pipermail/cfe-dev/2019-January/060919.html

[Quuxplusone]

(+jyknight)

Turns out we've discussed the "=m" situation last year already:
http://lists.llvm.org/pipermail/cfe-dev/2019-March/061742.html

According to that thread, "=m" implies that the argument must be overwritten by
the inline assembly, and if the assembly fails to do so, that's a bug in the
program, not in the compiler.
Therefore it should be safe to delete prior writes to the variable, if it's
marked as a "=m" output of inline assembly.

"+m" means that the argument is also being read, so prior writes should be left
untouched.

Adding "volatile" to the asm statement shouldn't really change anything, as
this doesn't make other writes to the output variable volatile.

Regarding "memory", I also think it shouldn't affect dead store elimination.
James, what do you think?

[Quuxplusone]

(In reply to Alexander Potapenko from comment #3)
> Regarding "memory", I also think it shouldn't affect dead store elimination.
> James, what do you think?

Doesn't memory mean it can read arbitrary memory, including memory that
overlaps with the output?  Or does it specifically exclude any memory that
overlaps with the output?

[Quuxplusone]

(In reply to Eli Friedman from comment #4)
> (In reply to Alexander Potapenko from comment #3)
> > Regarding "memory", I also think it shouldn't affect dead store elimination.
> > James, what do you think?
>
> Doesn't memory mean it can read arbitrary memory, including memory that
> overlaps with the output?  Or does it specifically exclude any memory that
> overlaps with the output?

I don't interpret the "memory" clobber on an asm block as saying anything about
what the asm block reads. The memory clobber says that an asm block may write
to memory, but isn't required to do so. A memory output operand OTOH gives the
asm block an obligation to fill the given memory memory, not just an option to
do so, so an output operand seems strictly stronger than a clobber. I agree
that the memory clobber shouldn't affect DSE.

[Quuxplusone]

While the constraints "=m", "+m", and "m" do make the address of the value
available to the assembly, they should be considered as passing and returning
*values* to the inline-asm "function", not addresses.

The "memory" clobber means it is allowed to both read *and* write to other
memory. That would only include the output value *if* the address had otherwise
escaped, and thus was reachable by the asm through other means than the "=m".

(Similarly, you cannot write to an "m" constraint, just because you add a
"memory" constraint.)

But, an asm probably should not remove the store if the address is already
escaped, AND the asm specifies "memory", e.g. a case like this:

extern int *px;
int bar()
{
  int x = 5;
  px = &x;
  asm("..." : "=m"(x) : : "memory");
  return foo(&x);
}

or, more directly:

int bar()
{
  int x = 5;
  asm("..." : "=m"(x) : "r"(&x) : "memory");
  return foo(&x);
}

GCC does _not_ appear to preserve the write to "x" in these two cases. IMO that
should be considered a bug.

But, we SHOULD be able to remove the store of 5, if the address isn't
accessible yet:
int bar()
{
  int x = 5;
  asm("..." : "=m"(x) : : "memory");
  return foo(&x);
}

or if the asm doesn't read/write other memory:

int bar()
{
  int x = 5;
  asm("..." : "=m"(x) : "r"(&x) : );
  return foo(&x);
}