[PATCH] D99784: [LICM] Hoist loads with invariant.group metadata

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek requested changes to this revision.
Prazek added a comment.

I had some time to look into it now, sorry that I missed it on first revision.
If I understand the LICM code correctly, it needs to drop all the instruction metadata when hoisting instruction that will not be executed unconditionally.  This means that code like:
https://godbolt.org/z/bjP64PbhY

  struct A {
    virtual void foo();
  };
  
  bool p(int, int);
  
  void loop(A& a, int n) {
  
    int i = 0;
    for (int i = 0 ; i < n; i++) {
        if (p(i, n))
            a.foo();
    }
  }

When vtable load will be hoisted, it will be stripped from invariant.group metadata.  This means that we won't gonna be able to devirtualize it further after e.g. inlining this function somewhere (exposing other virtual call or construction of `a`.
Assuming my understanding is right (please run this example with your patch), I would oppose to always hoist loads with !invariant.group loads.  It still might be beneficial for default optimization pipeline, but I worry that it will limit the optimizer across modules (LTO, ThinLTO etc).
I think it would be better to be on the safer side and hoist loads with !invariant.load only if they are executed unconditionally.  I tried to prototpe it long time ago here: https://reviews.llvm.org/D45151


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99784