This is an archive of the discontinued LLVM Phabricator instance.

[CFE] Add nomerge function attribute to inline assembly.
ClosedPublic

Authored by pengfei on Jul 21 2020, 2:15 AM.

Details

Summary

Sometimes we also want to avoid merging inline assembly. This patch add
the nomerge function attribute to inline assembly.

Diff Detail

Event Timeline

pengfei created this revision.Jul 21 2020, 2:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 2:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
This revision is now accepted and ready to land.Jul 21 2020, 11:57 AM
This revision was automatically updated to reflect the committed changes.
xbolva00 added inline comments.
clang/lib/Sema/SemaStmtAttr.cpp
186

This is totally wrong, just big hack to smuggle it here.

rnk added a comment.Feb 7 2022, 11:51 AM

I think LLVM already doesn't do some tail merging optimizations on inline asm, but allowing the use of the attribute is more principled, and will block more optimizations (CSE).

In D84225#3302142, @rnk wrote:

I think LLVM already doesn't do some tail merging optimizations on inline asm, but allowing the use of the attribute is more principled, and will block more optimizations (CSE).

IIRC, the initial requirment is to avoid the CSE like optimizations. We usually use inline asm for sepcial proposes. We have to stop the merge some time.

clang/lib/Sema/SemaStmtAttr.cpp
186

Could you explain more? Is there any unexpect sideeffect by this?

In D84225#3302142, @rnk wrote:

I think LLVM already doesn't do some tail merging optimizations on inline asm, but allowing the use of the attribute is more principled, and will block more optimizations (CSE).

IIRC, the initial requirment is to avoid the CSE like optimizations. We usually use inline asm for sepcial proposes. We have to stop the merge some time.

Since the big hammer (nomerge) is already there i suppose this is fine,
but given that there is little context in the original description,
the wording makes it seem like it's being used to workaround
something that may or may not be a bug in the first place.

There isn't anything inherently wrong with merging inlineasm in general,
if that does not break the constraints, especially since
there's already a sideeffect keyword possible on the inlineasm.

In D84225#3302142, @rnk wrote:

I think LLVM already doesn't do some tail merging optimizations on inline asm, but allowing the use of the attribute is more principled, and will block more optimizations (CSE).

IIRC, the initial requirment is to avoid the CSE like optimizations. We usually use inline asm for sepcial proposes. We have to stop the merge some time.

Since the big hammer (nomerge) is already there i suppose this is fine,
but given that there is little context in the original description,
the wording makes it seem like it's being used to workaround
something that may or may not be a bug in the first place.

There isn't anything inherently wrong with merging inlineasm in general,
if that does not break the constraints, especially since
there's already a sideeffect keyword possible on the inlineasm.

It's not a workaround. We do need to avoid the merging sometime. For example, given we have 2 branches begin with inline asm of endbr. We have to use nomerge to stop them been merged out of the branches. sideeffect doesn't help with that.

rnk added a comment.Feb 8 2022, 9:34 AM

It's not a workaround. We do need to avoid the merging sometime. For example, given we have 2 branches begin with inline asm of endbr. We have to use nomerge to stop them been merged out of the branches. sideeffect doesn't help with that.

That doesn't sound sufficient to ensure that endbr will be the first instruction in that basic block, which I'm guessing is a requirement. PHI nodes might cause register copies / spills to appear before endbr, and instrumentation passes typically insert code at the top of basic blocks. It sounds like we might need a more complete solution for tracking indirect branch target blocks. Maybe indirectbr and basic block addresses already feed into this, but I'm out of my depth here.

Anyway, I don't want to make a value judgment here. I'm in favor of this change. We should allow users to apply nomerge to inline asm, whether it is a workaround or not.

In D84225#3305140, @rnk wrote:

It's not a workaround. We do need to avoid the merging sometime. For example, given we have 2 branches begin with inline asm of endbr. We have to use nomerge to stop them been merged out of the branches. sideeffect doesn't help with that.

That doesn't sound sufficient to ensure that endbr will be the first instruction in that basic block, which I'm guessing is a requirement. PHI nodes might cause register copies / spills to appear before endbr, and instrumentation passes typically insert code at the top of basic blocks. It sounds like we might need a more complete solution for tracking indirect branch target blocks. Maybe indirectbr and basic block addresses already feed into this, but I'm out of my depth here.

Anyway, I don't want to make a value judgment here. I'm in favor of this change. We should allow users to apply nomerge to inline asm, whether it is a workaround or not.

Thanks @rnk . Yes, so we emit endbr in a backend pass rather than this way. I just want to demonstrate why we can't merge inline asm sometime. I don't have a better example at a short time :)

xbolva00 added inline comments.
clang/lib/Sema/SemaStmtAttr.cpp
186

It looks unfortunate to have something like AsmStmt in "CallExprFinder" with CallExpr as reference to clang's CallExpr.

Kinda surprised that your list of reviewers missed ALL known clang developers/code owner, in this case especially @aaron.ballman .

aaron.ballman added inline comments.Feb 15 2022, 5:47 AM
clang/lib/Sema/SemaStmtAttr.cpp
186

Yeah, I would have expected that something named CallExprFinder would only find call expressions, not use of inline assembly. The class now seems to be misnamed and that may be surprising to users. This is then being built on top of by things like https://reviews.llvm.org/D119061.

I'm not certain what a reasonable name for the class is given that we now want to use it for different purposes.

pengfei added inline comments.Feb 15 2022, 6:55 AM
clang/lib/Sema/SemaStmtAttr.cpp
186

Thanks @xbolva00 and @aaron.ballman for the input!
I added it to suppress the diagnosis and it's OK since it's the only use of the class at that time. I'm fine with the change on D119061.