[PATCH] D73763: [DSE] Lift post-dominance restriction.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1485
+        cast<Function>(M->getOrInsertFunction("____foobar", FnTy).getCallee());
+    State.ExitUseFn->addFnAttr(Attribute::ReadOnly);
+
----------------
jdoerfert wrote:
> I'm not sure this is allowed in a function pass.
Do you mean adding a new global or adding a function attribute to the added global? 

Ideally function passes would only modify things in the  function scope, but I think adding a new global is quite common, as most function passes that add new calls also may need to add a declaration of the called function.

As for adding the attribute, this needs to definitely change before submitting! I think we can either use an intrinsic (with the right attributes) or model it directly in MemorySSA, whatever option is preferred. 




================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1596
 
   // Find a MemoryDef writing to \p DefLoc and dominating \p Current, with no
   // read access in between or return None otherwise. The returned value may not
----------------
Tyker wrote:
> the comment seems outdated.
> i think that with this patch dominating will not be true anymore.
> and MemoryPHI seems to not cause a bail out.
Yes, I've rebased the patch and updated the comment as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73763