[PATCH] D81822: [DA] propagate loop live-out values that get used in a branch

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/lib/Analysis/DivergenceAnalysis.cpp:111-114
+  if (DeferredTerminators.count(&Term)) {
+    DeferredTerminators.erase(&Term);
+    return true;
+  }
----------------
foad wrote:
> I think it might be cleaner to put this in `compute` just before the call to `updateTerminator`. It feels wrong to make `updateTerminator` non-const, and the relationship between Worklist and DeferredTerminators seems clearer if instructions are removed from them in the same place, in `compute`. But I'll happily defer to others who are more familiar with this code.
Yeah, I was torn between what you describe and what is seen here in the patch. I don't have a strong opinion either way, so I just picked one. In its defense, the current version keeps the "detail" out of compute ... membership in the deferred set is just one of the criteria to decide whether to update the terminator. But I can see why removing the const is not palatable. 

One way to balance both sides is to  move the erase operation to propagateBranchDivergence, which is where the terminator is marked divergent anyway. This nicely separates the readonly part from the modify part. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81822