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

[Maxpm]

Summary

See: https://bugs.llvm.org/show_bug.cgi?id=42173

[andreadb]

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

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

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

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]

llvm/docs/CommandGuide/llvm-mca.rst

265–269

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]

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.

[andreadb]

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]

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.

[andreadb]

Looks very good.

Thanks Max!

[Maxpm]

Thanks, @andreadb.

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

[mattd]

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

llvm/docs/CommandGuide/llvm-mca.rst

# LLVM-MCA-BEGIN bar
     sub %eax, %edx
   # LLVM-MCA-END foo
     add %eax, %edx
   # LLVM-MCA-END bar
 
 Note that multiple anonymous regions cannot overlap. Also, overlapping regions
 cannot have the same name.
 
+USING MARKERS IN SOURCE CODE
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
 Inline assembly directives may be used from source code to annotate the
 assembly text:
 
 .. code-block:: c++
 
   int foo(int a, int b) {
     __asm volatile("# LLVM-MCA-BEGIN foo");
     a += 42;
     __asm volatile("# LLVM-MCA-END");
     a *= b;
     return a;
   }
 
+However, currently, `this interferes with optimizations like loop vectorization
+<https://bugs.llvm.org/show_bug.cgi?id=42173>`_ and changes the code that is generated.
+This is because the ``__asm`` statements are seen as real code having important side
+effects, which limits how the code around them can be transformed.
+
+It is generally safe to use source-level markers in straight-line source code without
+any control flow, like the example above.  Otherwise, you must take care to make
+sure that the code you are analyzing is the same as the code the compiler actually
+wants to generate.  Unfortunately, the only robust way to do this is to manually
+add the markers to the assembly text.
+
 HOW LLVM-MCA WORKS
 ------------------
 
 :program:`llvm-mca` takes assembly code as input. The assembly code is parsed
 into a sequence of MCInst with the help of the existing LLVM target assembly
 parsers. The parsed sequence of MCInst is then analyzed by a ``Pipeline`` module
 to generate a performance report.