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:
> > > 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?
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:158
+ Constant *Flag = nullptr;
+ auto TheFlag = SavedFlags.find(SP);
+ if (TheFlag == SavedFlags.end()) {
----------------
hans wrote:
> Having both Flag and TheFlag might be confusing. Could we rely on the
> DenseMap default-constructing values on lookup and do:
>
> ```
> Constant *&Flag = SavedFlags[SP];
> if (!Flag) {
> ...
> Flag = ...
> }
> ```
yep, this is better. Thanks.
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
