ychen 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: > 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? 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
