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

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added a comment.

In https://reviews.llvm.org/D45151#1113727, @hfinkel wrote:

> You say that you don't yet know if this is profitable yet. Do you have reason to believe that it might not be profitable (e.g., some example where it seems like we might want to constrain the behavior)? We almost never favor keeping metadata over doing other transformations, so I think it's worth being explicit about our reasoning here. Just saying "I don't know yet" is probably not sufficient, as we could say that about nearly all metadata, and in that case, change the default logic.


The reasoning behind it is that given function:

void foo(A *a) {

  other(a);
  while(...)
    something();
    a->bar();

}

we could hoist vtable and virtual function loads from the loop, but it would strip the metadata. 
This could be very bad after we would inline the function in the context where the dynamic type is known by invariant.group

void caller() {
 A a;
 foo(&a);
}

now because we stripped the metadata, and because call to "other" blocks us from const propagating vptr value we would not be able to determine a->bar.
If we would not hoist vptr load then by providing other store or load with invariant group we would be able to const propagte it to the one inside the loop.

I think that instead of hoisiting the vtable load all the time we should unroll the loop one time, so that vptr value can be propagated from the first iteration. This way we would still keep the metadata.

I did not yet collected benchmarking data with and without this patch, but looking at one microbenchmark from LNT that had speedup arround 70% I am pretty confident that can make very similar benchmark, but that would use the example that I showed to show that stripping metadata while hoisting vtable load would not be as beneficial.


Repository:
  rL LLVM

https://reviews.llvm.org/D45151