Page MenuHomePhabricator

Add nomerge function attribute to supress tail merge optimization in simplifyCFG
ClosedPublic

Authored by zequanwu on Apr 22 2020, 11:37 AM.

Details

Summary

We want to add a way to avoid merging identical calls so as to keep the separate debug-information for those calls. There is also an asan usecase where having this attribute would be beneficial to avoid alternative work-arounds.
Here is the link to the feature request: https://bugs.llvm.org/show_bug.cgi?id=42783.
nomerge is different from noline. noinline prevents function from inlining at callsites, but nomerge prevents multiple identical calls from being merged into one.

This patch adds nomerge to disable the optimization in IR level. A followup patch will be needed to let backend understands nomerge and avoid tail merge at backend.

Diff Detail

Event Timeline

zequanwu created this revision.Apr 22 2020, 11:37 AM
  1. Tests missing
  2. Why isn't noinline sufficient, why a whole new attribue?
zequanwu updated this revision to Diff 259455.Apr 22 2020, 7:23 PM
  1. Tests missing
  2. Why isn't noinline sufficient, why a whole new attribue?
  1. Test added.
  2. We want to avoid merging multiple call sites of same function, but noinline cannot achieve this. For example, we don't want to merge calls in i == 5 and i == 7 cases in the test case.
zequanwu updated this revision to Diff 259938.Apr 24 2020, 11:04 AM

I think that expanding the patch description will help motivate this and clarify the use-cases for which this is useful, and how it differentiates from noinline.
Perhaps include that the attribute aims to avoid merging identical calls so as to keep the separate debug-information for those calls, and that there is an asan usecase where having this attribute would be beneficial to avoid alternative work-arounds.
If you have a link to the feature request for this, include that in the description as well, or include the motivation behind it.

Note there are other LLVM passes (beside SimplifyCFG) that may need to check for this argument to prevent optimizations (e.g. LICM does instruction sinking/hoising, add check ~LICM.cpp:1092?).

Include a comment in the clang/test/CodeGen/attr-nomerge.c test on the expected outcome of the test. Something like:
"The attribute no-merge prevents the 3 bar() calls from being merged into one inside foo. Check that there are still 3 tail calls."

zequanwu edited the summary of this revision. (Show Details)Apr 24 2020, 4:24 PM
zequanwu updated this revision to Diff 260038.Apr 24 2020, 5:28 PM
zequanwu updated this revision to Diff 260039.Apr 24 2020, 5:37 PM

Add check in LICM to prevent sink or hoist on nomerge call

rnk added a comment.Apr 24 2020, 6:19 PM

Add check in LICM to prevent sink or hoist on nomerge call

Does sinking and hoisting remove the debug source location? I assumed that it wouldn't, but now after all the smooth stepping work, maybe it does.

Maybe we should audit for calls to DILocation::getMergedLocation, and use that to guide our changes. It would also be good to build an API that ensures that passes do not use getMergedLocation for instructions marked this way.


This also needs to be documented in llvm/docs/LangRef.rst with the other function attributes. Check out Arthur's preallocated patch for an example of that: D74651

clang/include/clang/Basic/AttrDocs.td
356–357 ↗(On Diff #260039)

I think we need to expand on this. This is user facing documentation visible here:
http://clang.llvm.org/docs/AttributeReference.html

Here's a suggestion:
"Calls to functions marked `nomerge` will not be merged during optimization. This attribute can be used to prevent the optimizer from obscuring the source location of certain calls. For example, it will prevent tail merging otherwise identical code sequences that raise an exception or terminate the program. Tail merging normally reduces the precision of source location information, making stack traces less useful for debugging. This attribute gives the user control over the tradeoff between code size and debug information precision."

zequanwu updated this revision to Diff 260529.Apr 27 2020, 7:13 PM

Split the the original differential to 2 smaller ones. This one add nomerge function attribute in IR level. Another one will add the attribute to let frontend recognize it.

Does sinking and hoisting remove the debug source location? I assumed that it wouldn't, but now after all the smooth stepping work, maybe it does.

Yes, both sinking and hoisting in simplifycfg remove the debug source location by Instruction::applyMergedLocation
sinking: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L1646
hoisting: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L1338

zequanwu updated this revision to Diff 260780.Apr 28 2020, 3:07 PM

Add test case for preventing hoisting.

rnk added a comment.Apr 29 2020, 2:26 PM

This looks pretty close, let's do one more iteration.

llvm/include/llvm/IR/InstrTypes.h
1718

Let's say "call" instead of "invoke". This method appears in the CallBase class. "Invoke" specifically refers to a call in LLVM IR which has an exceptional edge attached to it.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1305

It seems inconsistent that we don't apply the same logic to inline asm here. I think you should modify cannotMerge to return isInlineAsm() || hasFnAttr(...NoMerge), and then remove the isInlineAsm case below.

llvm/test/Transforms/SimplifyCFG/nomerge.ll
9

I would prefer retaining CHECK lines for the basic block labels to make it clear that these calls are all in separate blocks.

35

ditto regarding labels

zequanwu updated this revision to Diff 261059.Apr 29 2020, 3:11 PM
zequanwu marked 4 inline comments as done.

I think this looks good, but I would like to get a second opinion from Alina (@asbirlea).

asbirlea accepted this revision.May 4 2020, 3:57 PM

Thank you for adding this!

Please update first sentence in the description to:

We want to add a way to avoid merging identical calls so as to keep the separate debug-information for those calls. There is also an asan usecase where having this attribute would be beneficial to avoid alternative work-arounds.
This revision is now accepted and ready to land.May 4 2020, 3:57 PM
zequanwu edited the summary of this revision. (Show Details)May 4 2020, 4:22 PM
lebedev.ri added inline comments.Fri, May 8, 11:58 AM
llvm/include/llvm/IR/Attributes.td
106

Elsewhere we seem to be preventing any kind of movement,
not just disabling inlining of such functions if the call is a tailcall.

zequanwu updated this revision to Diff 262933.Fri, May 8, 12:48 PM
zequanwu marked an inline comment as done.

update comment.

zequanwu updated this revision to Diff 262977.Fri, May 8, 4:40 PM

add nomerge case in CodeExtractor.cpp

zequanwu marked an inline comment as done.Tue, May 12, 1:15 PM
zequanwu added inline comments.
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1305

This changes breaks the test AArch64/arm64-shrink-wrapping.ll because the inlineAsm cannot be deleted. I think we should only check for hasFnAttr(NoMerge) for CB1 and CB2.

zequanwu marked an inline comment as not done.Tue, May 12, 1:15 PM
This revision was automatically updated to reflect the committed changes.
rnk added inline comments.Wed, May 13, 1:30 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1305

Before pushing this patch, I undid the change to cannotMerge so that this stayed the same and the test passed.