OCHyams wrote: > Looks good with a test tweak and question. My understanding of what's going > on here is that: > > * Assignment-expressions that store a scalar to memory are added to the > "current" source atom, > * For source constructs that haven't been instrumented yet, that's > potentially a no-op as the atom group will be zero, > * In this scenario of the initialization of an automatic variable, the group > is set, and so the store gets the group number and rank.
That's right. > I suppose this is a good way of decoupling the code that identifies the > source construct that is "interesting" into the thing that calls > ApplyAtomGroup, and the code that actually instruments instructions into the > helper functions. That's got the upside that we don't have to > think-about-and-manipulate the AST past an "interesting" construct; with the > downside that things can leak out: > > * "Uncovered" stores that aren't in an atom group, or that /would/ be in a > "better" atom group but leak into an "outer" construct (as it were) > * Unexpectedly multiple stores entering an atom group?, It's possible yep. OTOH, there are times we want to do that on purposes (aggregate init), so it's an important use case. > * Atom groups that unexpectedly don't cover anything. This is indeed a fear, as there's many ways to create a store/store-like-intrinsic that don't go through the instrumented functions. This current stack takes the approach of trying to instrument every one of those sites. Another approach is to do it inside the builder class so that all call sites are automatically covered. Then instead of "opt in" we'd need a mechanism for some call sites to "opt out" which feels overall probably safer. I was going to play around with the idea once all these patches land, as we'll have good test coverage by then. > ...and thinking about all those different scenarios that can pop up, this > feels like a _good_ abstraction that avoids having to think about that. Maybe > we don't cover every single construct / combination that's expressible, but > this is all strictly an improvement in stepping anyway. --- I've addressed the inline comments (which have been eaten by github) - how does this look now? https://github.com/llvm/llvm-project/pull/134633 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
