Skip to content

[IRGen] Consume all arguments when generating Builtin.ZeroInitializer. #32530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

zoecarver
Copy link
Contributor

Before this patch, the Explosion destructor would assert because the arguments weren't consumed. This patch simply consumes all arguments in the BuiltinValueKind::ZeroInitializer case.

Refs #32402.

Before this patch, the Explosion destructor would assert because the arguments weren't consumed. This patch simply consumes all arguments in the BuiltinValueKind::ZeroInitializer case.
@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Jun 24, 2020
@zoecarver zoecarver requested review from gribozavr and jckarter June 24, 2020 19:35
@jckarter
Copy link
Contributor

I don't think this is correct. The explosion may contain further elements representing other values that are expected to be consumed after the zeroInitializer value. Why are we emitting a zeroInitializer call with an address in the first place? What do you expect that to do—produce a zero address, or zero out the referenced memory? The former would be undefined, but the latter would need a different address-only-type-specific implementation.

@zoecarver
Copy link
Contributor Author

zoecarver commented Jun 26, 2020

The explosion may contain further elements representing other values that are expected to be consumed after the zeroInitializer value.

Sorry, I'm not sure I follow. How would the explosion contain further elements? After adding a nulled out element for each member in the type, the function will always return. I think the expectation is that all arguments are consumed when the function (emitBuiltinCall) returns (but, I could be wrong about that).

What do you expect that to do—produce a zero address, or zero out the referenced memory? The former would be undefined, but the latter would need a different address-only-type-specific implementation.

We want to do the latter, zero out the referenced memory. What do you mean by an address-only-type-specific implementation? Why doesn't it work to generate a null value for each member's type?

@jckarter
Copy link
Contributor

Sorry, I'm not sure I follow. How would the explosion contain further elements? After adding a nulled out element for each member in the type, the function will always return. I think the expectation is that all arguments are consumed when the function (emitBuiltinCall) returns (but, I could be wrong about that).

There are places in IRGen where an Explosion contains elements related to multiple SILValues, particularly when working with arguments and returns, but possibly in other situations. Correct code should only consume the number of arguments it expects.

We want to do the latter, zero out the referenced memory. What do you mean by an address-only-type-specific implementation? Why doesn't it work to generate a null value for each member's type?

Until this point, zeroInitializer has only expected to be used with "loadable" types which can be represented in SIL and LLVM IR as direct values. As such, it's only implemented expecting to receive zero inputs and generate zero values as results. If you want it to work with address arguments, that will require a new implementation that takes an Address value, and memsets the referenced memory.

@zoecarver
Copy link
Contributor Author

There are places in IRGen where an Explosion contains elements related to multiple SILValues, particularly when working with arguments and returns, but possibly in other situations. Correct code should only consume the number of arguments it expects.

OK. Good to know.

Until this point, zeroInitializer has only expected to be used with "loadable" types which can be represented in SIL and LLVM IR as direct values. As such, it's only implemented expecting to receive zero inputs and generate zero values as results. If you want it to work with address arguments, that will require a new implementation that takes an Address value, and memsets the referenced memory.

Also good to know. I'll close this patch and make another that creates a memset when creating the constructor body in the clang importer.

@zoecarver zoecarver closed this Jun 26, 2020
@zoecarver
Copy link
Contributor Author

(I'll also create patches to update the other constructors not to use Builtin.ZeroInitializer for address types.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants