[PATCH] D118428: [clang-cl] Support the /JMC flag

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:145
+  LLVMContext &Ctx = M.getContext();
+  bool UseX86FastCall = Triple(M.getTargetTriple()).getArch() == Triple::x86;
+
----------------
ychen wrote:
> ychen wrote:
> > hans wrote:
> > > I still worry a bit about the target-specific code here. Normally, IR passes don't have any target-specific knowledge, but ask classes such as TargetTransformInfo for target-specific details, or possibly take them as input to the pass. For example, see llvm/lib/Transforms/CFGuard/CFGuard.cpp
> > > 
> > > I'm also not sure that lib/CodeGen/ is the right place for this pass, since most files there seem to be machine-IR passes. Maybe the natural place for this would be lib/Transforms/Instrumentation/? Is there some good pass we can compare this with?
> > > I still worry a bit about the target-specific code here. Normally, IR passes don't have any target-specific knowledge, but ask classes such as TargetTransformInfo for target-specific details, or possibly take them as input to the pass. For example, see llvm/lib/Transforms/CFGuard/CFGuard.cpp
> > Understood. `TargetTransformInfo` is mostly for the "IR optimization passes". The JMC pass is "IR codegen passes", it is more similar to `CodeGenPrepare` pass than any "IR optimization passes". I think we could move the target-specific stuff into the `TargetPassConfig` & its derived classes, not in this patch, but the following ELF port one. WDYT?
> > > I still worry a bit about the target-specific code here. Normally, IR passes don't have any target-specific knowledge, but ask classes such as TargetTransformInfo for target-specific details, or possibly take them as input to the pass. For example, see llvm/lib/Transforms/CFGuard/CFGuard.cpp
> > Understood. `TargetTransformInfo` is mostly for the "IR optimization passes". The JMC pass is "IR codegen passes", it is more similar to `CodeGenPrepare` pass than any "IR optimization passes". I think we could move the target-specific stuff into the `TargetPassConfig` & its derived classes, not in this patch, but the following ELF port one. WDYT?
> 
> Scratch that. I think this is more OS/platform-specific than target-specific. For X86, MSVC COFF and ELF are likely to have different symbol mangling and section naming preferences. And this information is pretty specific to JMC, like section name '.msvcjmc'. I think only X86 COFF has this `weird` mangling happen in LLVM codegen instead of the frontend. For non-x86 COFF and ELF, the handling is pretty much the same. So it may not be worth the effort of putting small pieces of OS/platform-specific information elsewhere?
I think the existing PGO passes do a variety of target-specific things, and they live in lib/Transforms/Instrumentation. For example, they pick different section names for ELF and COFF.

This seems like the function entry/exit instrumentation, and I wonder if it should be added as part of the CodeGenPassBuilder.h list of passes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118428/new/

https://reviews.llvm.org/D118428