zbrid added a comment. Thanks for putting this up! Here are a few comments.
================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:1 +//==========-- ImmutableGraph.h - A fast DAG implementation ---------=========// +// ---------------- Might be useful if you add a comment about what makes this a fast DAG impl in case someone may want to use it later. ================ Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:254 + + LLVM_DEBUG(dbgs() << "***** " << getPassName() << " : " << MF.getName() + << " *****\n"); ---------------- I think this should go at the top of the function. ================ Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:271 +// Apply the mitigation to `MF`, return the number of fences inserted. +// If `FixedLoads` is `true`, then the mitigation will be applied to both fixed +// and non-fixed loads; otherwise, only non-fixed loads. ---------------- Am I misunderstanding this comment? It sounds like if FixedLoads is true then BOTH fixed loads and non-fixed loads will be mitigated. Since runOnMachineFunction would call hardenLoads twice for non-fixed loads, would that result in double mitigation for non-fixed loads in the case where we also harden fixed loads? Unfortunately I'm having trouble reasoning through this myself, so I'd appreciate some clarification. ================ Comment at: llvm/test/CodeGen/X86/O0-pipeline.ll:61 +; CHECK-NEXT: Machine Dominance Frontier Construction +; CHECK-NEXT: X86 Load Value Injection (LVI) Load Hardening Pass ; CHECK-NEXT: Lazy Machine Block Frequency Analysis ---------------- Remove pass from name since that's typically the convention. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75936/new/ https://reviews.llvm.org/D75936 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
