sconstab 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));
----------------
craig.topper wrote:
> 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?
It had not been addressed, so thank you for pointing this out. That lambda was
doing too many things at once, which made it more confusing than it needed to
be. So I just inlined it in the
```
for (auto N : Uses) {
…
}
```
loop, and I added some additional clarifying comments.
================
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`
----------------
craig.topper wrote:
> mattdr wrote:
> > Some more detail would be useful here: precise about what? What are the
> > likely errors?
> >
> Was this answered somewhere?
This was referring to the use of `mayLoad()`. At the time I wrote that comment,
I wasn't sure that `mayLoad()` was exactly what was needed there, but I now
think that it does suffice (SLH also uses `MachineInstr::mayLoad()`).
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:409
+ NodeList Defs = SA.Addr->members_if(DataFlowGraph::IsDef, DFG);
+ llvm::for_each(Defs, AnalyzeDef);
+ }
----------------
craig.topper wrote:
> 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?
Wow, big oversight on my part. @mattdr was correct that this was doing a LOT of
extra work. I added a memoization scheme that remembers the instructions that
may transmit for each def. The getGadgetGraph() routine now runs about 75%
faster.
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
