[PATCH] D65280: Add a pass to lower is.constant and objectsize intrinsics

[Joerg Sonnenberger via Phabricator via llvm-commits]

joerg marked an inline comment as not done.
joerg added inline comments.


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:113-117
+        WeakTrackingVH InstPtr(&*I);
+        HasDeadBlocks = replaceWithSimplifyCFG(Inst, NewValue);
+        Changed = true;
+        if (!InstPtr || !I->getParent())
+          I = BB.begin();
----------------
chandlerc wrote:
> As I tried to indicate before, this can be even simpler AFAICT.
> 
> I think you should keep a single worklist of unsimplified users. Then you should just do the recursive inst simplify here using the same worklist each time. You'll want to change the loop to go in RPO over the function rather than in the naive order.
> 
> Then below this loop you will have a single worklist of unsimplified users that you can walk with the exact code you have above AFAICT.
Inside a BB, we have to use forward order for the processing so that the intrinsic calls are removed with the expected results. This means that the recursive simplification can remove the next instruction (InstPtr) or unhook it (no parent). Both cases are covered by the test cases. I don't see how a different iteration order inside the BB can work or avoid the problem.

Updating the CFG directly can help in those cases were PHIs are removed by removePredecessor. That seems generally desirable as property. So the question is which iteration order for the BBs to use.  BB iteration order can change the result, but I'm not sure computing RPO is worth the hassle here? I'm leaving the actual removing of the dead block to removeUnreachableBlocks, especially since I don't want to open-code recursive removal and other edge cases.


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

https://reviews.llvm.org/D65280