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

Wed Feb 2 22:08:54 PST 2022

ychen added inline comments.

================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:462
+  static cl::opt<bool> EnableJMCInstrument(
+      "enable-jmc-instrument",
+      cl::desc("Instrument functions with a call to __CheckForDebuggerJustMyCode"),
----------------
ychen wrote:
> hans wrote:
> > ychen wrote:
> > > hans wrote:
> > > > ychen wrote:
> > > > > hans wrote:
> > > > > > Other "on/off" options don't seem to have "enable" in the name or flag spelling, e.g. "-strict-dwarf", not "-enable-strict-dwarf". Maybe this should be just "-jmc-instrument" and JMCInstrument?
> > > > > The "-jmc-instrument" is already used by the pass itself (`#define DEBUG_TYPE "jmc-instrument"`). The `DEBUG_TYPE` one enables `opt -jmc-instrument`; this makes `llc -enable-jmc-instrument` to run the pass in IR codegen pipeline.
> > > > > 
> > > > > Just renamed `cl::opt<bool> EnableJMCInstrument` to `cl::opt<bool> JMCInstrument`.
> > > > Maybe the fact that -jmc-instrument is used by the IR pass is a hint that there shouldn't be an llc option for this, then? Looking at the other options here, they're all about codegen features, whereas the instrumentation in this patch really happens at a higher level.
> > > There are three kinds of passes (each in a separate pipeline), in the order of execution: IR optimization passes, IR codegen passes (some examples are here: https://github.com/llvm/llvm-project/blob/1d0244aed78114d5bd03ec4930d7687d6e587f99/llvm/include/llvm/CodeGen/MachinePassRegistry.def#L38-L51 and https://github.com/llvm/llvm-project/blob/1d0244aed78114d5bd03ec4930d7687d6e587f99/llvm/include/llvm/CodeGen/MachinePassRegistry.def#L106-L121) and MIR codegen passes. The JMC pass is an IR codegen passes. It runs within the codegen phase, but it transforms IR and it is tested using the `opt` tool. The llc option is for testing that the pass could be enabled using `TargetOptions::JMCInstrument` (clang uses this), the change in `llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp` and this option enables LTO+JMC with `lld -mllvm -enable-jmc-instrument`. 
> > Thanks for the pointer, the IR codegen passes is something I'm not very familiar with.
> > 
> > Just looking at a few of the first ones you linked to, I see that they live under lib/Transforms/. And when I look in lib/CodeGen/ the vast majority of those work on MachineFunctions -- but not all. So I'm not really sure how we decide what should go where?
> > 
> > I think the best way is to look for a similar pass and see how that works. Maybe llvm/lib/Transforms/CFGuard/CFGuard.cpp could be a good example, since it's also inserting Windows-specific instrumentation at the IR level, similar to this pass.
> Looking into the CFGuard passes, I think they are all "IR codegen passes". I have no idea why they need to stay in lib/Transform (and in their own library `LLVMCFGuard`) instead of in lib/CodeGen. I would argue that they should probably be in `lib/CodeGen` instead. Hi @rnk , any idea about this?
Any objection if I keep the JMC pass in CodeGen and move CFGuard pass to CodeGen in a separate patch?

================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:145
+  LLVMContext &Ctx = M.getContext();
+  bool UseX86FastCall = Triple(M.getTargetTriple()).getArch() == Triple::x86;
+
----------------
rnk wrote:
> 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.
 > 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.

Yeah, it should. CodeGenPassBuilder.h is already lagging behind TargetPassConfig by far. I could take care of it when making `CodeGenPassBuilder.h` have parity with TargetPassConfig in terms of all lit tests.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118428