[PATCH] D83595: [Draft][MSAN] Optimize away poisoning allocas that are always written before load

[Gui Andrade via Phabricator via llvm-commits]

guiand added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3948
+
+    // Traverse to the Node's successors if we haven't yet
+    for (auto Node = succ_begin(BB); Node != succ_end(BB); Node++) {
----------------
eugenis wrote:
> guiand wrote:
> > guiand wrote:
> > > eugenis wrote:
> > > > I don't think this is entirely correct. Consider this:
> > > > ```
> > > > entry { alloca 32; store 0..16 } -> A, B
> > > > A { store 16..32 } -> B
> > > > B { load }
> > > > ```
> > > > traversing entry->A->B will mark B as done even though there is a path where control flow arrives at B with alloca not entirely initialized.
> > > > 
> > > > What happens to the benchmarks if this algorithm is limited to a single basic block?
> > > I tried to write the code so that it would look to see if *all* paths are initialized before a load. So in your example, it would look at the path `entry -> A -> B`, and it would see that it's fully stored, and it would continue looking at the next path. For `entry -> B`, `if (!firstUsesAreStore) return false` would come into effect, and we won't optimize away the poisoning.
> > Oh, I see where the issue is here: the `if TraversedSet.count` check. I needed some way to prevent cycles in the graph, so that was the first thing I reached for, but you're right, this approach breaks the algorithm.
> Right. And it does not even require a cycle.
> 
> This is a dataflow problem - assign labels to graph nodes showing which bytes are always initialized at entry, and update them based on predecessor's labels until the fixed point is reached. But in this case, I suspect, most of the optimization opportunities are within a single BB.
I think this could be fixed by removing the target node from the set after the recursive call is finished. But I'll measure the impact of removing the inter-block stuff entirely to see how much value it adds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83595/new/

https://reviews.llvm.org/D83595