craig.topper added inline comments.
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:322
+ DenseSet<std::pair<GraphIter, GraphIter>> GadgetEdgeSet;
+ auto AnalyzeUse = [&](NodeAddr<UseNode *> Use, MachineInstr *MI) {
+ assert(!(Use.Addr->getFlags() & NodeAttrs::PhiRef));
----------------
mattdr wrote:
> Please add a comment explaining the semantics of the boolean return here. I
> //think// it's: `true` if we need to consider defs of this instruction
> tainted by this use (and therefore add them for analysis); `false` if this
> instruction consumes its use
Was this comment addressed?
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:341
+ GadgetBegin.first, GadgetEnd.first);
+ if (UseMI.mayLoad()) // FIXME: This should be more precise
+ return false; // stop traversing further uses of `Reg`
----------------
mattdr wrote:
> Some more detail would be useful here: precise about what? What are the
> likely errors?
>
Was this answered somewhere?
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:409
+ NodeList Defs = SA.Addr->members_if(DataFlowGraph::IsDef, DFG);
+ llvm::for_each(Defs, AnalyzeDef);
+ }
----------------
mattdr wrote:
> We analyze every def from every instruction in the function, but then
> //also// in `AnalyzeDefUseChain` analyze every def of every instruction with
> an interesting use. Are we doing a lot of extra work?
Was this answered somewhere?
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
