IRGen: enable the dynamic cast optimization on Windows

[compnerd]

This accommodates lazy initialization of class metadata for the dynamic
cast elision optimization. In the case of a lazy initialization, we
cannot assume that we are able to directly access the metadata through
a GEP, instead we should go through the type metadata accessor.
Although this is a slight bit more expensive, the hot-path of this is
roughly a function call + pointer check (if the value is in the cache,
we just return, otherwise we will have to initialize the type metadata
the first time around).

Special thanks to @atrick for the discussion, pointers, and general help
with trying to enable this optimization!

Resolves SR-16112.

[compnerd]

@swift-ci please build toolchain Windows platform

[compnerd]

@swift-ci please test

[atrick]

@compnerd I'm happy you figured out a fix.

@eeckstein implemented lazy witness tables, so he'll know better if the fix is right

First, how did this bug manifest? Did a type check simply fail when it should have succeeded, or do we actually fail to initialize a VWT? (In the crash info, I didn't notice any incorrect type checks, but I also don't understand how this optimization could result in uninitialized metadata)

Since I don't know the details of metadata initialization in IRGen, I'll ask these questions to the general audience for my own understanding...

if (IGF.IGM.IRGen.Opts.LazyInitializeClassMetadata)

Is this isa-check optimization really dependent on ClassMetadataStrategy? Should we check ClassMetadataStrategy::Fixed vs. ClassMetadataStrategy::Singleton instead of an IRGen option? And bail completely for unhandled strategies?

In the case of ClassMetadataStrategy::Singleton does IRGenModule::getAddrOfTypeMetadata still return an absolute symbol reference? If that's just a bogus symbol that should never be linked against, then it seems like that should be a linker error rather than allowing runtime code to reference the wrong metadata.

[atrick]

It seems that we should no longer call getAddrOfTypeMetadata for lazy initialized metadata. It looks like dead code to me.

[compnerd]

No, it is not dead code - it is simply unnecessary work. It is the "else" case that is being hoisted and done unconditionally. I can sink it into an else clause.

[compnerd]

First, how did this bug manifest? Did a type check simply fail when it should have succeeded, or do we actually fail to initialize a VWT? (In the crash info, I didn't notice any incorrect type checks, but I also don't understand how this optimization could result in uninitialized metadata)

The type check would give the wrong result, though the VWT would be emitted, but not be complete. The changes here will force a call to the metadata accessor. This will ensure that we have the metadata initialized (as the accessor does exactly that - either initialize the metadata or return it).

Is this isa-check optimization really dependent on ClassMetadataStrategy? Should we check ClassMetadataStrategy::Fixed vs. ClassMetadataStrategy::Singleton instead of an IRGen option? And bail completely for unhandled strategies?

This is the crux of the issue that I feel I do not understand :). This might be overly conservative of a check - the metadata strategy would be best to consult I think but comes at a slight penalty - a call + cmp.

In the case of ClassMetadataStrategy::Singleton does IRGenModule::getAddrOfTypeMetadata still return an absolute symbol reference? If that's just a bogus symbol that should never be linked against, then it seems like that should be a linker error rather than allowing runtime code to reference the wrong metadata.

I believe it is not a linker issue - the linker cannot identify that the symbol should not be used - it is a runtime decision.

[eeckstein]

Thanks for investigating this issue!
lgtm

[compnerd]

@eeckstein thanks for the review! @atrick I'm going to merge this for now to clear up the build, but I'm happy to revisit and improve things stylistic or otherwise.

[atrick]

Thanks again @compnerd.

Here's my follow-up request:

if (IGF.IGM.IRGen.Opts.LazyInitializeClassMetadata) {

This check is misleading because the problem has nothing to do with lazy initialization. You're using this as a poxy for "is this a COFF/PE binary?". That can be fixed with a comment. In that comment, can you explain the issue with "emitting the GEP" to the extent that you understand it? Is the problem that we can't directly emit a reference to a global metadata symbol, presumably because we need to load it from the GOT? Or is the problem that the compiler cannot compute a constant offset from that symbol (since computing an "isa" from a metadata symbol requires an adjustment for the VWT entry)? It's strange to me that LLVM can't handle this. @jckarter do you have any insight here?

llvm::Value *destMetadata = IGF.IGM.getAddrOfTypeMetadata(targetFormalType);

I don't understand why you're still emitting the unused "invalid GEP" on Windows. Presumably LLVM strips it out....

[compnerd]

You're using this as a poxy for "is this a COFF/PE binary?". That can be fixed with a comment. In that comment, can you explain the issue with "emitting the GEP" to the extent that you understand it? Is the problem that we can't directly emit a reference to a global metadata symbol, presumably because we need to load it from the GOT? Or is the problem that the compiler cannot compute a constant offset from that symbol (since computing an "isa" from a metadata symbol requires an adjustment for the VWT entry)? It's strange to me that LLVM can't handle this. @jckarter do you have any insight here?

Well, other targets can also force the lazy initialization of the class metadata. On Windows it is required because we cannot form the reference statically as we need to indirect through the moral equivalent of the GOT (called the IAT or the import address table). As a result we always use the singleton strategy to construct the metadata to deal with pointers that may lie outside of the module (e.g. $sBoW). By using the accessor for the metadata we ensure that it is initialized properly.

LLVM cannot handle this because there is no way to represent a pointer outside the module. The address will be determined at runtime, and you can find it as *__imp_<symbol>.

If there is a better solution, I'm happy to adjust this.

[compnerd]

I don't understand why you're still emitting the unused "invalid GEP" on Windows. Presumably LLVM strips it out....

Oh, I see the confusion here now that you had. Yes, it is "dead IR" on Windows. I can certainly sink that into an explicit else case rather than rely on LLVM dead stripping the unnecessary GEP. Sorry I was not connecting the IR generation and actual logic and so failed to understand the concern.