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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D99784#2749459 <https://reviews.llvm.org/D99784#2749459>, @rnk wrote:

> Thanks for the repro!
>
>   while (true) {
>     std::string FPath = StrCat(dir, "index", Idx++, "/");
>     std::ifstream f(StrCat(FPath, "size"));
>     if (!f.is_open()) break;
>     std::string suffix;
>     int size;
>     f >> size;
>     if (f.fail()) // BUS error
>       return;
>     if (f.good()) {
>       std::cout << "read " << size << "\n";
>     }
>   }
>
> Clang is able to devirtualize the `fail` call on its own. However, it still has IR to perform the virtual base class adjustment:
>
>   %13 = bitcast %"class.std::basic_ifstream"* %f to %"class.std::basic_istream"*
>   %call3 = call nonnull align 8 dereferenceable(16) %"class.std::basic_istream"* @_ZNSirsERi(%"class.std::basic_istream"* nonnull dereferenceable(16) %13, i32* nonnull align 4 dereferenceable(4) %size)
>   %14 = bitcast %"class.std::basic_ifstream"* %f to i8**
>   %vtable = load i8*, i8** %14, align 8, !tbaa !10, !invariant.group !12
>   %vbase.offset.ptr = getelementptr i8, i8* %vtable, i64 -24
>   %15 = bitcast i8* %vbase.offset.ptr to i64*
>   %vbase.offset = load i64, i64* %15, align 8
>   %16 = bitcast %"class.std::basic_ifstream"* %f to i8*
>   %add.ptr = getelementptr inbounds i8, i8* %16, i64 %vbase.offset
>   %17 = bitcast i8* %add.ptr to %"class.std::basic_ios"*
>   %call4 = call zeroext i1 @_ZNKSt9basic_iosIcSt11char_traitsIcEE4failEv(%"class.std::basic_ios"* nonnull dereferenceable(264) %17)
>
> This vtable load is based directly on the `%f` alloca, and not the result of a launder operation. Because there is no launder operation, LICM hoists this vptr load to the entry block, so it loads uninit stack memory. The virtual base class offset load happens before calling `fail`, which is where the crash happens.
>
> So: this is a frontend bug, and perhaps a frontend missed optimization. In general, clang does not launder local variable allocas, and that seems like it could be a problem. However, always laundering and stripping every dynamic local variable is probably way overkill, and would block SROA. Maybe we could only use laundered object pointers for the vptr loads.

I think the current frontend implementation is correct at least in this instance, the load from the alloca should always result in the same value. Although I'm wondering if it's possible that we end up using one alloca for different dynamic types with different lifetimes. If two different types have the invariant.group metadata on vtable loads, that would be bad.

The problem is that we're moving the load before the store (or call to constructor).
Currently, LICM will hoist loads, but only if it can prove the memory they point to is not modified in the loop. This patch bypasses that check for loads with invariant.group metadata. So the current patch is still wrong. We can only hoist if the corresponding store is outside the loop. With invariant.group metadata we're not worried about the loop changing the value, just that the value was stored to in the first place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99784