[opt] Re-work broadenSingleElementStores to use projections. #32318
Conversation
zoecarver
force-pushed
the
opt/project-broaden
branch
from
dda13fc to
5ad9ee4
Compare
| // loadable type, we can't create it as a struct later on. | ||
| // If our aggregate has unreferenced storage then we can never prove if it | ||
| // actually has a single field. | ||
| if (!baseAddrType.isLoadable(*f) || |
There was a problem hiding this comment.
@atrick what do you think about supporting opaque types here?
There was a problem hiding this comment.
What cases are you trying to handle by doing that? Can you find out if it's possible to have
baseAddrType.isLoadable(*f) == true
AND
baseAddrType.aggregateHasUnreferenceableStorage() == false
??
There was a problem hiding this comment.
Do you mean baseAddrType.isLoadable(*f) && baseAddrType.aggregateHasUnreferenceableStorage() == true? If not, I think that's most loadable types.
I think if we have a trivial struct with a bitfield that would do it: struct Foo { int x : 3; };. That struct will be loadable (even though, maybe it shouldn't be) but will also have un-referenceable storage (x).
There was a problem hiding this comment.
ok, so what case are you trying to handle? A struct with a single opaque property? So that "store" would need to actually be a copy_addr?
There was a problem hiding this comment.
@atrick my understanding is that has unreferencable storage is for things from c/c++ that have memory/other things that are not exposed to swift. To me it is sort of like an early, hacky resilience-esque sort of thing.
There was a problem hiding this comment.
what case are you trying to handle?
I'm not trying to handle any particular case. Currently, we will bail on opaque types. I just wanted to make sure that's the correct behavior.
To me it is sort of like an early, hacky resilience-esque sort of thing.
I think this is true, yes. But most non-loadable types are non-loadable for ABI reasons, or to make sure calling conventions work with the C++ definitions.
There was a problem hiding this comment.
isLoadable seems like a sensible bailout considering we're dealing with storing values at its address
|
I don't want to say that I carefully reviewed this or thought about alternatives, but the implementation looks reasonable to me. |
| // loadable type, we can't create it as a struct later on. | ||
| // If our aggregate has unreferenced storage then we can never prove if it | ||
| // actually has a single field. | ||
| if (!baseAddrType.isLoadable(*f) || |
There was a problem hiding this comment.
@atrick my understanding is that has unreferencable storage is for things from c/c++ that have memory/other things that are not exposed to swift. To me it is sort of like an early, hacky resilience-esque sort of thing.
* Fixes loadable edge case for address-only types. * Re-work, generalize, and simplify by using projections. * Support `-enable-cxx-interop` in sil-opt.
zoecarver
force-pushed
the
opt/project-broaden
branch
from
5ad9ee4 to
b695aee
Compare
|
@gottesmm thanks for the review. I addressed all your comments. |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test. |
|
@swift-ci please test windows platform. |
| @@ -0,0 +1,18 @@ | |||
| #ifndef VALIDATION_TEST_SILOPTIMIZER_FOO_H | |||
There was a problem hiding this comment.
@zoecarver Can you change the name of this file to something more meaningful? I didn't notice the name when I was reviewing.
-enable-cxx-interopin sil-opt.Refs #32291.