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

statement_toIR(): simplify conversion of case statements to goto #10322

Merged
merged 1 commit into from Dec 12, 2019

Conversation

WalterBright
Copy link
Member

Remove dependency (and memory leak) of hash table it used to use.

@WalterBright WalterBright added Easy Review Refactoring No semantic changes to code labels Aug 19, 2019
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10322"

@WalterBright
Copy link
Member Author

The semaphoreci failure is execution expired, whatever that unhelpful message means. Probably environmental.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 20, 2019

Both extra as a field name, and the comment aren't really very descriptive.

This also brings back to the fore the argument of why add more backend fields that are used only by dmd, when gdc/ldc can't have their own? (#5501).

Couldn't this be added to a backend type?

Perhaps it's time to think again about how best to encapsulate backend types in frontend headers. The last rejected attempt was to move all backend fields into a hash table (#4569).

And infact you are effectively reverting #4854 here, which flies in the face of why that patch was made in the first place. To remove dmd baggage from the frontend.

@WalterBright
Copy link
Member Author

@ibuclaw I'm actually going to need it to implement the flow analysis for the future ownership/borrow checker, which will be part of the front end. Doing it first for s2ir will minimize the scope of the changes.

@dnadlinger
Copy link
Member

But surely that flow analysis would actually use frontend types?

@WalterBright
Copy link
Member Author

But surely that flow analysis would actually use frontend types?

It'll be a different type for the DFA than for s2ir, but the code will be very similar. At this point I don't know if they'll be similar enough to combine, we'll have to see about that.

@thewilsonator
Copy link
Contributor

All of this seems to me, like we are trying to dance around the fact that we don't have a high-level IR as part of the compilation process, unlike say swift, rust, julia, the Stupid D Compiler, etc.

I guess I'll be able to say with more confidence if the SAoC project I proposed gets chosen (it is concerned with (principally) optimisation with a multilevel IR).

@dnadlinger
Copy link
Member

@thewilsonator: Wouldn't such an IR still have the same problem, that is, how to attach backend-specific data to AST nodes?

@thewilsonator
Copy link
Contributor

I don't think so, since "translation to the backend format" would then be an IR-to-IR transform and therefore no need to worry about the AST at all at that point. The IR would come with a notion of e.g. a block, and then the backend would translate that to the block its corresponding IR.

@dnadlinger
Copy link
Member

dnadlinger commented Aug 20, 2019

By "AST nodes" I meant IR nodes of whatever form. Isn't the key concern here wanting to attach extra analysis information to the backend input nodes? This would presumably apply just the same if the backend glue layer consumes some desugared IR as it is now when working on frontend AST nodes.

@WalterBright
Copy link
Member Author

we are trying to dance around the fact that we don't have a high-level IR as part of the compilation process, unlike say swift, rust, julia, the Stupid D Compiler, etc.

The AST is the high level IR.

By "AST nodes" I meant IR nodes of whatever form.

AST means Abstract Syntax Tree. IR nodes are their own language, and are not representing a syntax. An AST can be an IR, but not all IRs are ASTs.

if the backend glue layer consumes some desugared IR as it is now when working on frontend AST nodes

This is really not the way to go. The front end AST is designed to be user-friendly so, for example, error messages can be related to the language syntax. The back end form is designed to be math friendly - amenable to proofs, optimization, graph theory, and code generation. It is not something a user would want to see error messages from.

The glue layer's job is to convert between the two.

In order to implement Ownership/Borrowing, DFA has to be done. But since error messages emanate from it, it has to be in AST form. So my strategy is to layer another data structure over the AST which is amenable to AST. All I need to make that work efficiently is the two extra PIMPL fields I added. GDC and LDC can probably make effective use of them as well for the same reason.

@thewilsonator
Copy link
Contributor

The AST is the high level IR.

Sorry I meant to imply SSA IR.

In order to implement Ownership/Borrowing, DFA has to be done.

Yes, and this is most easily done in SSA.

But since error messages emanate from it, it has to be in AST form.

Couldn't be further from the truth. All that is required for good error messages is source location and a message that says (1) what went wrong, (2) why, and (3) a enough information to inform the user how to fix that class of error. None of that is predicated on having an AST.

One of the major goals and features of MLIR is source location tracking so that error messages are of high informational utility and accurately reflect the origin of the error, and they are/do.

@WalterBright
Copy link
Member Author

Many error messages print out the failing expression. Can't do that without the AST.

@thewilsonator
Copy link
Contributor

Sure you can, you'd need source ranges rather than source locations. Its about time we did something to reduce the size of Loc anyway, thinking about source ranges at the same time would be a good move.

@dlang-bot dlang-bot merged commit b1221c0 into dlang:master Dec 12, 2019
@WalterBright WalterBright deleted the extraCase branch December 12, 2019 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Easy Review Refactoring No semantic changes to code
Projects
None yet
6 participants