This is an archive of the discontinued LLVM Phabricator instance.

Attach readonly and readnone attributes to inline-asm instructions
ClosedPublic

Authored by ahatanak on Jun 18 2015, 12:55 PM.

Details

Summary

Currently, clang/llvm handles inline-asm instructions in a very conservative manner, choosing not to eliminate the instructions or hoisting them out of a loop even when it's safe to do so. This patch makes changes to clang to attach a readonly or readnone attribute to an inline-asm instruction, which enables passes such as LICM and EarlyCSE to move or optimize away the instruction.

The patch is based on the patch Chad proposed in 2012:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057131.html

If this patch is approved, I plan to add another test case to check CSE and LICM can remove an inline-asm or hoist it out of a loop if readonly or readnone is attached.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 27952.Jun 18 2015, 12:55 PM
ahatanak retitled this revision from to Attach readonly and readnone attributes to inline-asm instructions.
ahatanak updated this object.
ahatanak edited the test plan for this revision. (Show Details)
ahatanak added a subscriber: Unknown Object (MLST).
ahatanak updated this revision to Diff 28317.Jun 23 2015, 7:17 PM

Made changes to the previous patch to stop attaching readonly or readnone in the following cases:

  1. The inline-asm has no output operands. In this case, it is implicitly volatile.
  2. The inline-asm returns a value by-reference instead of by-value. The previous patch wasn't catching the case where a "=r" constraint resulted in a value being returned by-reference.

Also, fixed the test case to catch those two cases.

hfinkel added a subscriber: hfinkel.Jul 9 2015, 7:18 PM

Does this mean that if I have two inline asm statements, once which sets a condition-code/model-specific register, and one which reads that register, because neither read or write memory, we'd now be allowed to reorder them? I'm concerned that, because we use memory dependencies to track all dependencies, this will lead to us losing track of other register dependencies that we should otherwise track.

Do the inline-asm statements you are thinking of use clobber register list to represent the dependency between the first and second statement? If they are using input and output operands or the volatile keyword, clang will know they can't be reordered, so I was guessing they were using the clobber list?

Do the inline-asm statements you are thinking of use clobber register list to represent the dependency between the first and second statement? If they are using input and output operands or the volatile keyword, clang will know they can't be reordered, so I was guessing they were using the clobber list?

I take back what I said, mostly. GCC's documentation says that, "GCC's optimizers sometimes discard asm statements if they determine there is no need for the output variables. Also, the optimizers may move code out of loops if they believe that the code will always return the same result (i.e. none of its input values change between calls). Using the volatile qualifier disables these optimizations. asm statements that have no output operands, including asm goto statements, are implicitly volatile."

So we're okay here, such inline asm statements need to be marked volatile, or have no output registers, so we can check for that.

I don't think, however, you can ever mark the inline asm call as 'readnone', because you have no way of knowing that it does not load anything. Any of its inputs could be an address, or used to construct an address, to something. The programmer might also know that the address is always dereferenceable, and thus won't produce any side effects other than its result (and, thus, not mark the statement as volatile).

We could parse the assembly using the integrated assembler to figure this out, but that's another story.

I don't think, however, you can ever mark the inline asm call as 'readnone', because you have no way of knowing that it does not load anything. Any of its inputs could be an address, or used to construct an address, to something. The programmer might also know that the address is always dereferenceable, and thus won't produce any side effects other than its result (and, thus, not mark the statement as volatile).

The gcc documentation has the following sentence:

The "memory" clobber tells the compiler that the assembly code performs memory reads or writes to items other than those listed in the input and output operands (for example, accessing the memory pointed to by one of the input parameters).

I took it to mean that you have to add "memory" to an inline-asm statement's clobber list in the case it reads from memory using one of the input registers. If you don't, gcc will treat it as a read-none statement.

asm ("movl (%1), %0" : "=r" (res) : "r" (ptr) : "memory");

hfinkel accepted this revision.Jul 10 2015, 9:56 AM
hfinkel added a reviewer: hfinkel.

I don't think, however, you can ever mark the inline asm call as 'readnone', because you have no way of knowing that it does not load anything. Any of its inputs could be an address, or used to construct an address, to something. The programmer might also know that the address is always dereferenceable, and thus won't produce any side effects other than its result (and, thus, not mark the statement as volatile).

The gcc documentation has the following sentence:

The "memory" clobber tells the compiler that the assembly code performs memory reads or writes to items other than those listed in the input and output operands (for example, accessing the memory pointed to by one of the input parameters).

Okay, sounds good.

I took it to mean that you have to add "memory" to an inline-asm statement's clobber list in the case it reads from memory using one of the input registers. If you don't, gcc will treat it as a read-none statement.

asm ("movl (%1), %0" : "=r" (res) : "r" (ptr) : "memory");

In that case, LGTM.

This revision is now accepted and ready to land.Jul 10 2015, 9:56 AM
This revision was automatically updated to reflect the committed changes.