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"),
----------------
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`.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:145
+ LLVMContext &Ctx = M.getContext();
+ bool UseX86FastCall = Triple(M.getTargetTriple()).getArch() == Triple::x86;
+
----------------
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?
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:147
+
+ auto GetOrCreateCheckFunctionDecl = [&, Decl = FunctionCallee()]() mutable {
+ if (Decl)
----------------
hans wrote:
> This kind of mutable lambda feels pretty subtle to me. I think something like
> the below would be more straight-forward.
>
> ```
> Function *CheckFunction = nullptr;
> for (...) {
> ...
> if (!CheckFunction) {
> // Create the decl here.
> }
> }
> ```
>
> But if you strongly prefer the lambda, I suppose it's okay.
yep, sounds good.
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
