Thanks for the patch.
I am of the opinion that the change should be much smaller than this. See my commets below.
|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.
|265–269 ↗||(On Diff #203663)|
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....
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?
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.
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?