[PATCH] D45151: [LICM] Hoisting invariant.group loads

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added a comment.

In D45151#1200094 <https://reviews.llvm.org/D45151#1200094>, @hfinkel wrote:

> In D45151#1199948 <https://reviews.llvm.org/D45151#1199948>, @sanjoy wrote:
>
> > I'm sorry I can't be more decisive here since I wasn't deeply involved with the devirtualization work early on and so lack a lot of context.  The general direction here seems fine to me -- given that we use metadata to express a large range of things, it seems ok to have metadata specific MD dropping policies.  However, given that Hal originally objected to this, we should make sure he is on board.
>
>
> I understand the motivation, thanks. The problem here is that, while we might want to avoid hoisting so that we don't strip the metadata, that only is better if we happen to inline into a function that then provides a concrete type. Otherwise, we should have hoisted.  How about this: Add a parameter to LICM to control this choice: We can choose not to hoist during the early runs on LICM (which happen during inlining), and then hoist later (during the LICM invocation that runs after loop unrolling).


Sorry for late response, didn't have time to look into this.
I though about this and I would like to stick to the current implementation.  I think that having different LICM behavior introduces complexity that is not worth the cost.
We would also need to introduce new named passes, like licm-pre-inline and licm-after-inline.  It is also not clear what we should do after inlining when doing LTO.

I would stick to this approach, as it does not pessimize current implementation -- in some cases we can hoist vtable loads, but all of the vtable loads that would be hoisted before are also hoisted right now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D45151