[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.

In D109114#3007693 <https://reviews.llvm.org/D109114#3007693>, @kubamracek wrote:

>> Can the code here (matching ptrtoint/sub) be unified with the code that matches it in getPointerAtOffset, so those patterns won't diverge?
>
> Suggestions how to do that? The code in getPointerAtOffset is quite different, because is recurses *down into* the expression, whereas here we need to walk up by looking at users. I could move the code to be next to each other, would that be good enough?

Yeah moving the matcher code to a function might help, as well as moving them next to each other.

>> the assert here seems dangerous, as nothing guards against PtrExpr having multiple users, right? PtrExpr having multiple users is valid IR, so we should probably handle this gracefully.
>
> I believe the assert actually holds. At this point, we know that F is *only* referenced from vtables (either one or more) and nothing else (otherwise it wouldn't be in DeadFunctions). F can have multiple users, if there's multiple vtables, but each use is a Constant/ConstantExpr in a vtable. Unless I misunderstand something, I believe in a ConstantExpr like `trunc(sub(ptrtoint(x),ptrtoint(y))` all the nodes always have a unique single user and that's the parent ConstantExpr. Therefore the ptrtoint here, which must be in the vtable expression, must always have a single user.

Sounds good, can you add a message/comment to the assert?

In D109114#3007695 <https://reviews.llvm.org/D109114#3007695>, @kubamracek wrote:

>> Would it be possible to either only iterate over all users if we know that there are relative pointers or iterate over all uses once and either replace the sub with zero or the use with nullptr?
>
> Attempted to do this, but it complicates the implementation. Not sure if the extra complexity is worth it (we only save an extra iteration over vtables, and presumably in realistic programs that's a limited amount of those).

Agreed that it might not be too important, I'm fine with either.


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

https://reviews.llvm.org/D109114