This is an archive of the discontinued LLVM Phabricator instance.

Fix nomerge attribute not working with __builtin_trap().
Needs ReviewPublic

Authored by zequanwu on Mar 15 2023, 1:00 PM.

Details

Reviewers
hans
craig.topper
Summary
  1. It fixes the problem that llvm.trap() not getting the nomerge attribute.
  2. It sets nomerge flag for the node if the instruction has nomerge arrtibute.

Fixes #53011

Diff Detail

Event Timeline

zequanwu created this revision.Mar 15 2023, 1:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2023, 1:00 PM
zequanwu requested review of this revision.Mar 15 2023, 1:00 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 15 2023, 1:00 PM
zequanwu edited the summary of this revision. (Show Details)Mar 15 2023, 1:02 PM
hans added a comment.Mar 16 2023, 6:17 AM

Since we're touching SelectionDAG, does GlobalISel also need updating?

clang/test/CodeGen/attr-nomerge.cpp
44

Maybe do __debugbreak() too since that's also mentioned on the debug.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6852

(I guess you could have used DAG.getRoot() instead of creating the Node variable.)

6855

Do we need to handle this "else" branch too?

Or would it make sense to do the nomerge check in the caller of visitIntrinsicCall instead?

zequanwu updated this revision to Diff 505837.Mar 16 2023, 9:16 AM
zequanwu marked 3 inline comments as done.
  • Add test case for __debugbreak().
  • Handle named trap instructions and add test case for it.
clang/test/CodeGen/attr-nomerge.cpp
44

The __debugbreak() is also emitted from EmitTrapCall(). So, just add a test case for __debugbreak().

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6855

Setting CLI.NoMerge instead of setting DAG.getRoot(), because it later somehow replace the root with another instruction which causes the call instruction to lose the attribute.

Added a test case for else branch.

rsmith added a subscriber: rsmith.Mar 16 2023, 12:07 PM

FYI: this attribute appears to not work on (at least) x86 and arm currently, because the backend also does some merging: https://godbolt.org/z/d43M83oax

clang/lib/CodeGen/CGExpr.cpp
3626–3627

There are 496 calls to Builder.CreateCall in clang's CodeGen. Do they all need this change? If not, how can we be confident we've found all the ones that do? (From a quick check, at least MSVC's __fastfail builtin seems like it would also benefit from this handling.)

Would it be reasonable to make clang's CGBuilder do this for every call instruction we create?

FYI: this attribute appears to not work on (at least) x86 and arm currently, because the backend also does some merging: https://godbolt.org/z/d43M83oax

Thanks for reporting it. I'll look into it later.

clang/lib/CodeGen/CGExpr.cpp
3626–3627

I think all of them (builtin functions) should have that change, theoretically, because they are not different from __builtin_trap. Probably we should change the Builder.CreateCall to take a list of attributes that will be added to that CallInst and update all calls to CreateCall. Is there any easier approach?