This is an archive of the discontinued LLVM Phabricator instance.

[Docs] [llvm-mca] Point out a caveat for using llvm-mca markers in source code.
ClosedPublic

Authored by Maxpm on Jun 7 2019, 6:52 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Maxpm created this revision.Jun 7 2019, 6:52 PM

Hi Max,

Thanks for the patch.

I am of the opinion that the change should be much smaller than this. See my commets below.

Cheers
-Andrea

llvm/docs/CommandGuide/llvm-mca.rst
244–246 ↗(On Diff #203663)

I don't think that we should introduce a subsection here. I would rather keep the description of source level markers short and still as part of the same section.

260–263 ↗(On Diff #203663)

Please remove the reference to the upstream bugzilla. I don't think that we should add references to upstream bugs in this document.

Please change "changes the code that is generated" to "may have an impact on the code generated".

265–269 ↗(On Diff #203663)

The impact of source level markers on the compiler generated code is not quantifiable. It really depends on a number of things. So we shouldn't claim that there are cases where it is generally safe. It is misleading and inaccurate at best.

Even wanting to keep that first statement, it is unclear what you meant by "source-level markers in straight-line source code without any control flow". That is going to cause a lot of confusion to the reader (which knows already that a code sequence should never span through multiple basic blocks) and we don't want that.

Strictly speaking, the entire paragraph can be summarized as "use markers at your own risk", which is clearly already impled by your previous paragraph. So, to be honest it is a bit of a repetition.
In my opinion you should remove this entire paragraph, as it is redundant.

Maxpm marked an inline comment as done.Jun 8 2019, 10:16 AM
Maxpm added inline comments.
llvm/docs/CommandGuide/llvm-mca.rst
265–269 ↗(On Diff #203663)

The impact of source level markers on the compiler generated code is not quantifiable...So we shouldn't claim that there are cases where it is generally safe.

Yes, I see your point. If we don't suggest safe source constructions, could we at least suggest checking out the -Rpass-missed options so the user could double-check for herself? Or do you think even that is too unreliable?

Basically, I'm worried that the docs introduce this feature only to immediately and totally disavow it. If there's really no way to use it properly, should we be telling people how to use it at all? Then again, inline asm statements are something that someone could plausibly come up with on their own, so they should be warned of the gotcha....

the reader (which knows already that a code sequence should never span through multiple basic blocks)

Do they? I can't find that explicitly said in the docs anywhere, and the tool only warns when it finds a ret instruction. That might be worth another patch?

Maxpm updated this revision to Diff 203705.Jun 8 2019, 12:03 PM

Removed the subsubheading, second paragraph, and issue tracker link. Also removed the word "currently," since it's unlikely that the markers, in this particular form, will someday be free of this problem.

Maxpm marked 2 inline comments as done.Jun 8 2019, 12:03 PM

I understand your point.

The reality is that source level markers have never been a real feature in llvm-mca. What is described in that section as "source level marker" is simply just a "hack" to workaround the lack of a real feature. While I agree that it has mostly worked quite well in practice (at least so far), it is known to interfere with the optimizers.

So here is the idea: what if we change the paragraph that introduces source level markers by clarifying those points?

Something along the line of:

There is no support for source level markers in llvm-mca.
However, as a workaround, users can insert inline assembly as follows [example here]. [...] this may interfere with the optimizers [... your paragraph goes here ...]. If users want to make use of inline assembly to emit markers in the generated assembly, then the recommendation is to always verify that the output assembly is equivalent to the assembly generated in the absence of markers.

Essentially, we clarify that the feature is missing. We also don't set too high the expectation on the asm workaround. In that context, I think it is fine to suggest using -Rpass as one possible way to check missed optimization opportunities caused by the presence of inline assembly.

What do you think?

-Andrea

Maxpm updated this revision to Diff 203711.Jun 8 2019, 3:38 PM

Changed the description to explicitly refer to the inline __asm markers as a workaround, rather than a feature. Added a link to the -Rpass options in the Clang documentation.

Maxpm marked an inline comment as done.Jun 8 2019, 3:38 PM
andreadb accepted this revision.Jun 10 2019, 3:27 AM

Looks very good.

Thanks Max!

This revision is now accepted and ready to land.Jun 10 2019, 3:27 AM
Maxpm added a comment.EditedJun 10 2019, 11:43 AM

Thanks, @andreadb.

I don't have commit access. Could someone else commit this for me?

This revision was automatically updated to reflect the committed changes.
mattd added a comment.Jun 11 2019, 8:57 AM

Thanks for the patch @Maxpm! Can we close PR42173? Is there anything left for that ticket?