Skip to content
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

Move mangled name out of IRGlobalValue #752

Merged

Conversation

tangent-vector
Copy link
Contributor

Previously the IRGlobalValue type was used as a root for all IR instructions that can have "linkage," in the sense that a definition in one module can satisfy a use in another module.
The mangled symbol name was stored in state directly on each IRGlobalValue, which created some complications, and also forced IR instructions that wanted to support linkage to wedge into the hierarchy at that specific point.

This change moves the mangled name out into a decoration: either an IRImportDecoration or an IRExportDecoration, both of which inherit from IRLinkageDecoration which exposes the mangled name.
This change has a few benefits:

  • We can now have any kind of instruction be exported/imported, without having to inherit from IRGlobalValue. This could potentially let IRStructType and IRWitnessTable be simplified to just have operand lists instead of dummy chldren as they do today.

  • We can now easily have "global values" like functions that explicitly don't get linkage, instead of using a null or empty mangled name as a marker.

  • We can use the exact opcode on a linkage decoration to distinguish imports from exports, which could be used to more accurately resolve symbols during the linking step.

Other than adding the decorations and making sure that AST->IR lowering adds them, the main changes here are around any code that used IRGlobalValue. Variables and parameters of type IRGlobalValue* were changed to IRInst* easily, so the main challenge was around code that casts to `IRGlobalValue*.

In cases where a cast to IRGlobalValue also performed a test for the mangled name being non-null/non-empty, we simply switched the code to check for the presence of an IRLinkageDecoration, since that is the new way of indicating a value with linakge.

Most of the serious complications arose in ir.cpp around the "linking"/target-specialization and generic specialization steps.

The "linking" logic was checking for IRGlobalValue to opt into some more complicated cloning logic, and just checking for a linkage decoration here wasn't sufficient since the front-end does produce global values without linkage in some cases (e.g., for a function-static variable we produce a global variable without linkage). This logic was updated to just check for the cases that used to amount to IRGlobalValues directly by opcode. It might be simpler in the short term to have kept IRGlobalValue around to make the existing casts Just Work, but I'm confident that this logic could actually be rewritten for much greater clarity and simplicity and that is the better way forward.

The generic specialization logic was using some really messy code to generate a new mangled name to represent the specialized symbol, and then searching for an existing match for that name.
The original idea there was that an IR module could include "pre-specialized" versions of certain generics to speed up back-end compilation by eliminating the need to specialize in some cases, but this feature has never been implemented so the overhead here is just a waste.
Instead, I moved generic specialization to use a simpler dictionary to map the operands to a specialize instruction over to the resulting specialized value.
This allows for some simplifications in the name mangling logic, because it no longer needs to figure out how to produce mangled names from IR instructions representing types/values.

As part of this change I also overhauled the IR emit logic to produce cleaner output by default, borrowing some of the ideas from the logic in emit.cpp. IR values are now automatically given names based on their "name hint" decoration, if any, to make the code easier to follow, and I also made it so that types and literals get collapsed into their use sites in a new "simplified" IR dump mode (which is currently the default, with no way to opt into the other mode without tweaking the code). The resulting IR dumps are much nicer to look at, but as a result the one test that involves IR dumping (ir/string-literal) doesn't really test what it used to.

One weird issue that came up during testing is that the transitive-interface test had previously been producing output that made no sense (that is, the expected output file wasn't really sensible), and somehow these changes were altering its behavior. Changing the test to use int values instead of float was enough to make the output be what I'd expect, and hand inspection of generating DXBC has me convinced we were compiling the float case correctly too. There appears to be some issue around tests with floating-point outputs that we should investigate.

Previously the `IRGlobalValue` type was used as a root for all IR instructions that can have "linkage," in the sense that a definition in one module can satisfy a use in another module.
The mangled symbol name was stored in state directly on each `IRGlobalValue`, which created some complications, and also forced IR instructions that wanted to support linkage to wedge into the hierarchy at that specific point.

This change moves the mangled name out into a decoration: either an `IRImportDecoration` or an `IRExportDecoration`, both of which inherit from `IRLinkageDecoration` which exposes the mangled name.
This change has a few benefits:

* We can now have any kind of instruction be exported/imported, without having to inherit from `IRGlobalValue`. This could potentially let `IRStructType` and `IRWitnessTable` be simplified to just have operand lists instead of dummy chldren as they do today.

* We can now easily have "global values" like functions that explicitly *don't* get linkage, instead of using a null or empty mangled name as a marker.

* We can use the exact opcode on a linkage decoration to distinguish imports from exports, which could be used to more accurately resolve symbols during the linking step.

Other than adding the decorations and making sure that AST->IR lowering adds them, the main changes here are around any code that used `IRGlobalValue`. Variables and parameters of type `IRGlobalValue*` were changed to `IRInst*` easily, so the main challenge was around code that *casts* to `IRGlobalValue*.

In cases where a cast to `IRGlobalValue` also performed a test for the mangled name being non-null/non-empty, we simply switched the code to check for the presence of an `IRLinkageDecoration`, since that is the new way of indicating a value with linakge.

Most of the serious complications arose in `ir.cpp` around the "linking"/target-specialization and generic specialization steps.

The "linking" logic was checking for `IRGlobalValue` to opt into some more complicated cloning logic, and just checking for a linkage decoration here wasn't sufficient since the front-end *does* produce global values without linkage in some cases (e.g., for a function-`static` variable we produce a global variable without linkage). This logic was updated to just check for the cases that used to amount to `IRGlobalValue`s directly by opcode. It might be simpler in the short term to have kept `IRGlobalValue` around to make the existing casts Just Work, but I'm confident that this logic could actually be rewritten for much greater clarity and simplicity and that is the better way forward.

The generic specialization logic was using some really messy code to generate a new mangled name to represent the specialized symbol, and then searching for an existing match for that name.
The original idea there was that an IR module could include "pre-specialized" versions of certain generics to speed up back-end compilation by eliminating the need to specialize in some cases, but this feature has never been implemented so the overhead here is just a waste.
Instead, I moved generic specialization to use a simpler dictionary to map the operands to a `specialize` instruction over to the resulting specialized value.
This allows for some simplifications in the name mangling logic, because it no longer needs to figure out how to produce mangled names from IR instructions representing types/values.

As part of this change I also overhauled the IR emit logic to produce cleaner output by default, borrowing some of the ideas from the logic in `emit.cpp`. IR values are now automatically given names based on their "name hint" decoration, if any, to make the code easier to follow, and I also made it so that types and literals get collapsed into their use sites in a new "simplified" IR dump mode (which is currently the default, with no way to opt into the other mode without tweaking the code). The resulting IR dumps are much nicer to look at, but as a result the one test that involves IR dumping (`ir/string-literal`) doesn't really test what it used to.

One weird issue that came up during testing is that the `transitive-interface` test had previously been producing output that made no sense (that is, the expected output file wasn't really sensible), and somehow these changes were altering its behavior. Changing the test to use `int` values instead of `float` was enough to make the output be what I'd expect, and hand inspection of generating DXBC has me convinced we were compiling the `float` case correctly too. There appears to be some issue around tests with floating-point outputs that we should investigate.
if (otherSize > thisSize)
return false;

return UnownedStringSlice(begin(), begin() + otherSize) == other;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An adjacent point but think we might want to change the name of UnownedStringSlice -> StringSlice, and StringSlice -> OwnedStringSlice. That OwnedStringSlice can then derive from StringSlice, and all of the non length changing string like functionality can be placed in StringSlice, and delegated to from String.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that works, since OwnedStringSlice could still use the raw pointers from UnownedStringSlice and just add the retained String to that, so long as we are sure that the actual underlying allocation for the String can't move while we have a retained pointer to it (which I suppose is guaranteed).

if(valueCount != other.m_values.Count()) return false;
for(UInt ii = 0; ii < valueCount; ++ii)
{
if(m_values[ii] != other.m_values[ii]) return false;
Copy link
Contributor

@jsmall-zzz jsmall-zzz Dec 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test it says only identical instruction pointers are 'the same' for purposes of a key. But we could have to IRConstant (for example - or perhaps more complex things) that were the same value which should be the same instanciation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current status of the compiler is a bit off from ideal here, but the test in this case represents what I want it to be in the long run.

Rhe IRBuilder (through SharedIRBuilder) goes to some lengths to try to ensure that it doesn't create distinct IRConstant nodes that have the same type and value. For that matter, it also tries to make sure that it doesn't create two different IRType nodes that both represent the same time.

There are places in the code, however, where we might create new IRConstants or types as part of some pass, and those would only end up being unique'd within the SharedIRBuilder used by the given pass, and might not be globally unique. When that happens, we could accidentally instantiate a generic more than once given logic like that above.

In the case of generic specialization this isn't expected to happen in practice because the only step that precedes generic specialization is the "linking" step that is creating fresh IR (using a single SharedIRBuilder) by copying from other modules. That step already needs to deduplicate things because different modules can obviously have different constants with the same value.

Still, in the long run we either need to make the deduplication more global (so that it belongs to the IRModule and we somehow deal with issues around remove/deletion of instructions that might also be cached in a dictionary), or introduce an explicit pass to remove duplicates which we invoke before any pass that really cares about deduplication.

What I do not want to do is introduce more complex "structural" equality rules for things like this. At least not if there is any way to avoid it...

@tangent-vector tangent-vector merged commit 822ed70 into shader-slang:master Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants