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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
