[PATCH] D109114: [GlobalDCE] In VFE, replace the whole 'sub' expression of unused relative-pointer-based vtable slots

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the update. Still LGTM, but please wait a day or 2 with committing in case there are additional comments.



================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:204
+    if (auto *PtrExpr = dyn_cast<ConstantExpr>(U)) {
+      if (PtrExpr->getOpcode() == Instruction::PtrToInt) {
+        for (auto *PtrToIntUser : PtrExpr->users()) {
----------------
kubamracek wrote:
> fhahn wrote:
> > it might be worth turning some of those checks into early continues instead, to reduce the nesting level.
> Yes. Much better now :)
I think you could also combine both checks `if (!PtrExpr || PtrExpr->getOpcode() !=...)` but that's a super tiny nit.


================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:206
+        for (auto *PtrToIntUser : PtrExpr->users()) {
+          if (auto *SubExpr = dyn_cast<ConstantExpr>(PtrExpr->user_back())) {
+            if (SubExpr->getOpcode() == Instruction::Sub) {
----------------
kubamracek wrote:
> kubamracek wrote:
> > fhahn wrote:
> > > oh and just to double check, the assert was removed intentionally?
> > Yes, I dropped it intentionally, and replaced it with a loop over all users to be more conservative.
> > 
> > Actually, I did end up seeing the assert fail (!) in my testing on a large codebase (compiling the Swift stdlib with VFE on), but I couldn't figure out how to isolate the exact IR that could reproduce the problem. I guess I should try harder and add it as a lit testcase...
> Added "GlobalDCE/call-with-ptrtoint.ll" that triggers the assert -- in this case, we end up with a dead function that has a ptrtoint user, which itself has zero users (!), which I assume is an expected artifact of using GlobalDCE dropping all bodies of dead functions upfront. If unexpected, then it sounds like a separate bug.
> 
> In either case, I think dropping the assert and just iterating over all users of PtrExpr is the right approach here. It also makes this helper function more robust (as it doesn't assume the assert to hold).
Ok thanks for clarifying & adding the test.


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

https://reviews.llvm.org/D109114