[SIL] NFC: Shrink SILGlobalVariable by 16 bytes

[davezarzycki]

There are two changes here. One is simply moving a boolean. The other involves a specialization of llvm::Optional for SILLocation. This seems to tantalizingly close. When optimizations are enabled, only two tests fail but without optimizations, the Glibc swift module fails to build. Before I debug this further, is this a good idea? This technically collapses a level of optionality.

[eeckstein]

lgtm

[davezarzycki]

Thanks @eeckstein – I was concerned because it looks like llvm::Optional doesn't specialize pointers, which seems like both an obvious optimization and an obvious correctness goal.

[gribozavr]

Is llvm::Optional intended to be specialized like this? I'm concerned we would be forking its API inadvertently, because nothing forces us to keep this copy in sync.

[davezarzycki]

Hi @gribozavr – Ya, that's my concern as well and the lack of a pointer specialization was hint that llvm::Optional isn't designed to be specialized (which is disappointing). Who might know the answer?

[jckarter]

I share @gribozavr's concern. Maybe we can just not use Optional for the storage, and wrap it in Optional via an accessor instead.

[davezarzycki]

That was my first choice, but got shy of that approach because it required making SILLocation() public and I figured it was private for a reason. Am I wrong?

[jckarter]

We could maybe expose it as a static SILLocation invalid() method.

[rjmccall]

We could maybe expose it as a static SILLocation invalid() method.

Yeah, I like that plan better than exposing a default constructor.

[davezarzycki]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - ffc254570364dd60a5b8e4003f3d0dfc2fc2c3d8

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - ffc254570364dd60a5b8e4003f3d0dfc2fc2c3d8

[davezarzycki]

Oops. I had to force push an update. I realized after the fact that setLocation() wasn't updating the HasLocation boolean, but as it turns out nothing calls said function, so I just deleted it and made the SILLocation const.

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - da60232cf5c3220127bb97cae62a994ac4c03abb

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - da60232cf5c3220127bb97cae62a994ac4c03abb

A new approach

[davezarzycki]

Ping. Now that this has been simplified to the suggested SILLocation::invalid() approach, can I get a LGTM? Thanks :-)

[gribozavr]

@davezarzycki IDK who might know the answer, but since I can't easily find such specializations in LLVM codebase, I think it is safe to say it is not supported. If you are up for a templates programming adventure, you could refactor llvm::Optional to have a traits-based design (like dense map traits) that will allow to optimize the storage without forking the public API.

[davezarzycki]

Ya, a "extra inhabitants" trait (like Swift's internal logic for Swift code) would make llvm::Optional far more efficient.

I'm still looking for a LGTM sign off when somebody has the time. In particular, the const'ifying of the SILLocation given that nothing is updating the location after the SILGlobalVariable is created.

[gribozavr]

This PR looks good to me, but unfortunately I'm not familiar with this code sufficiently to be comfortable actually approving -- sorry!

[davezarzycki]

Ping @jckarter, @eeckstein. Might I get a LGTM review?

[rjmccall]

LGTM