[Starter Project] Teach SILParser how to parse switch_enum of undef.


(Michael Gottesman) #1

This is a small starter project for those who are interested in working with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote a patch that does the work some time ago, but I never have had time to finish it (i.e. make sure everything works ok/write tests). I posted the patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may serve as good starting point for someone who wants to poke at the SIL Parser (a part of the code base that has not gotten as much attention as others).

Michael


(Emanuel Zephir) #2

Unless anyone objects, I'd like to claim this. I've assigned issue SR-210
to myself.

--Emanuel

···

On Sun, Dec 13, 2015 at 1:29 PM, Michael Gottesman via swift-dev < swift-dev@swift.org> wrote:

This is a small starter project for those who are interested in working
with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote
a patch that does the work some time ago, but I never have had time to
finish it (i.e. make sure everything works ok/write tests). I posted the
patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may
serve as good starting point for someone who wants to poke at the SIL
Parser (a part of the code base that has not gotten as much attention as
others).

Michael

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev


(Davide C. C. Italiano) #3

I'm interested in something like this. I noticed this project has been
already taken, but you mentioned in a subsequent mail you have a list of
them. What are the other proposals?

Thank you,

···

On Sun, Dec 13, 2015 at 4:29 PM, Michael Gottesman via swift-dev < swift-dev@swift.org> wrote:

This is a small starter project for those who are interested in working
with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote
a patch that does the work some time ago, but I never have had time to
finish it (i.e. make sure everything works ok/write tests). I posted the
patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may
serve as good starting point for someone who wants to poke at the SIL
Parser (a part of the code base that has not gotten as much attention as
others).

Michael

Hi Michael,

--
Davide


(Michael Gottesman) #4

SGTM. If you want as you finish these, I have a list of them = ).

I just filed another one:

https://bugs.swift.org/browse/SR-247

Michael

···

On Dec 16, 2015, at 5:30 AM, Emanuel Zephir <emanuelzephir@gmail.com> wrote:

Unless anyone objects, I'd like to claim this. I've assigned issue SR-210 to myself.

--Emanuel

On Sun, Dec 13, 2015 at 1:29 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
This is a small starter project for those who are interested in working with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote a patch that does the work some time ago, but I never have had time to finish it (i.e. make sure everything works ok/write tests). I posted the patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may serve as good starting point for someone who wants to poke at the SIL Parser (a part of the code base that has not gotten as much attention as others).

Michael

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev


(Michael Gottesman) #5

Here are some off the top of my head. Many are related to code quality. Tell me what you think:

1. We have recently gotten some SILParser crashers. These need to be fixed. I looked at 1-2 of them and the ones I looked at are where we should be giving out a diagnostic but are instead just asserting.

2. The SILParser is a recursive descent parser. In general when one implements a recursive descent parser, one should above the routine that does the various parts of the parsing show the grammar being parsed. SILParser has inconsistently done this. I would go through all of the methods and see which are missing this documentation and fix the documentation.

3. parseSILInstruction is horrible and makes me cry every time I see it. It is a method that is ~1900 lines with a huge switch in it. We should refactor it into a visitor structure. In fact it is large enough that we should consider moving it into its own file if it is possible.

Michael

···

On Dec 22, 2015, at 12:02 PM, Davide Italiano <dccitaliano@gmail.com> wrote:

On Sun, Dec 13, 2015 at 4:29 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
This is a small starter project for those who are interested in working with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote a patch that does the work some time ago, but I never have had time to finish it (i.e. make sure everything works ok/write tests). I posted the patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may serve as good starting point for someone who wants to poke at the SIL Parser (a part of the code base that has not gotten as much attention as others).

Michael

Hi Michael,
I'm interested in something like this. I noticed this project has been already taken, but you mentioned in a subsequent mail you have a list of them. What are the other proposals?

Thank you,

--
Davide


(Emanuel Zephir) #6

Okay, that works.

I have a few questions about SR-210:

1) The attached patch adds support for undefined integer value-cases in the
switch_value instruction. What is the runtime meaning of this construct?

2) Are there any other SIL instructions in the select/switch family that
need modifications? If yes, which? At least some of them (e.g.
select_value) don't support this either.

3) Are there any areas that need special attention when writing tests and
otherwise validating this change?

--Emanuel

···

On Wed, Dec 16, 2015 at 10:15 AM, Michael Gottesman <mgottesman@apple.com> wrote:

SGTM. If you want as you finish these, I have a list of them = ).

I just filed another one:

https://bugs.swift.org/browse/SR-247

Michael

On Dec 16, 2015, at 5:30 AM, Emanuel Zephir <emanuelzephir@gmail.com> > wrote:

Unless anyone objects, I'd like to claim this. I've assigned issue SR-210
to myself.

--Emanuel

On Sun, Dec 13, 2015 at 1:29 PM, Michael Gottesman via swift-dev < > swift-dev@swift.org> wrote:

This is a small starter project for those who are interested in working
with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote
a patch that does the work some time ago, but I never have had time to
finish it (i.e. make sure everything works ok/write tests). I posted the
patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may
serve as good starting point for someone who wants to poke at the SIL
Parser (a part of the code base that has not gotten as much attention as
others).

Michael

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev


(Michael Gottesman) #7

You know what, let me file some bugs. I just thought of some more.

···

On Dec 22, 2015, at 1:45 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:

Here are some off the top of my head. Many are related to code quality. Tell me what you think:

1. We have recently gotten some SILParser crashers. These need to be fixed. I looked at 1-2 of them and the ones I looked at are where we should be giving out a diagnostic but are instead just asserting.

2. The SILParser is a recursive descent parser. In general when one implements a recursive descent parser, one should above the routine that does the various parts of the parsing show the grammar being parsed. SILParser has inconsistently done this. I would go through all of the methods and see which are missing this documentation and fix the documentation.

3. parseSILInstruction is horrible and makes me cry every time I see it. It is a method that is ~1900 lines with a huge switch in it. We should refactor it into a visitor structure. In fact it is large enough that we should consider moving it into its own file if it is possible.

Michael

On Dec 22, 2015, at 12:02 PM, Davide Italiano <dccitaliano@gmail.com <mailto:dccitaliano@gmail.com>> wrote:

On Sun, Dec 13, 2015 at 4:29 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
This is a small starter project for those who are interested in working with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote a patch that does the work some time ago, but I never have had time to finish it (i.e. make sure everything works ok/write tests). I posted the patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may serve as good starting point for someone who wants to poke at the SIL Parser (a part of the code base that has not gotten as much attention as others).

Michael

Hi Michael,
I'm interested in something like this. I noticed this project has been already taken, but you mentioned in a subsequent mail you have a list of them. What are the other proposals?

Thank you,

--
Davide

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev


(Joe Groff) #8

Okay, that works.

I have a few questions about SR-210:

1) The attached patch adds support for undefined integer value-cases in the switch_value instruction. What is the runtime meaning of this construct?

SIL 'undef' has the same meaning as LLVM's 'undef'. It means that the exact value isn't important and can be substituted with an arbitrary bit pattern. It's UB if the behavior of the program depends on the value of an undef, but safe if the undef can be eliminated, such as if it's dead, or eliminated by an operation like 'x & 0' or 'x - x' that's invariant of 'x'.

2) Are there any other SIL instructions in the select/switch family that need modifications? If yes, which? At least some of them (e.g. select_value) don't support this either.

'undef' ought to be parsed as part of the SIL value grammar. It'd be worth doing a pass to make sure we consistently use the same parsing rule everywhere we accept a value.

-Joe

···

On Dec 17, 2015, at 3:37 AM, Emanuel Zephir via swift-dev <swift-dev@swift.org> wrote:

3) Are there any areas that need special attention when writing tests and otherwise validating this change?

--Emanuel

On Wed, Dec 16, 2015 at 10:15 AM, Michael Gottesman <mgottesman@apple.com <mailto:mgottesman@apple.com>> wrote:
SGTM. If you want as you finish these, I have a list of them = ).

I just filed another one:

https://bugs.swift.org/browse/SR-247

Michael

On Dec 16, 2015, at 5:30 AM, Emanuel Zephir <emanuelzephir@gmail.com <mailto:emanuelzephir@gmail.com>> wrote:

Unless anyone objects, I'd like to claim this. I've assigned issue SR-210 to myself.

--Emanuel

On Sun, Dec 13, 2015 at 1:29 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>>wrote:
This is a small starter project for those who are interested in working with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote a patch that does the work some time ago, but I never have had time to finish it (i.e. make sure everything works ok/write tests). I posted the patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may serve as good starting point for someone who wants to poke at the SIL Parser (a part of the code base that has not gotten as much attention as others).

Michael

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev


(Michael Gottesman) #9

Okay, that works.

I have a few questions about SR-210:

1) The attached patch adds support for undefined integer value-cases in the switch_value instruction. What is the runtime meaning of this construct?

2) Are there any other SIL instructions in the select/switch family that need modifications? If yes, which? At least some of them (e.g. select_value) don't support this either.

3) Are there any areas that need special attention when writing tests and otherwise validating this change?

I would suggest creating a SIL test case, reading it into sil-opt, then taking the output from sil-opt and rerun it through sil-opt, then FileCheck that.

That will check:

1. Can we parse undef here.
2. Do we print undef here correctly.

Michael

···

On Dec 17, 2015, at 5:37 AM, Emanuel Zephir <emanuelzephir@gmail.com> wrote:

--Emanuel

On Wed, Dec 16, 2015 at 10:15 AM, Michael Gottesman <mgottesman@apple.com <mailto:mgottesman@apple.com>> wrote:
SGTM. If you want as you finish these, I have a list of them = ).

I just filed another one:

https://bugs.swift.org/browse/SR-247

Michael

On Dec 16, 2015, at 5:30 AM, Emanuel Zephir <emanuelzephir@gmail.com <mailto:emanuelzephir@gmail.com>> wrote:

Unless anyone objects, I'd like to claim this. I've assigned issue SR-210 to myself.

--Emanuel

On Sun, Dec 13, 2015 at 1:29 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
This is a small starter project for those who are interested in working with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote a patch that does the work some time ago, but I never have had time to finish it (i.e. make sure everything works ok/write tests). I posted the patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may serve as good starting point for someone who wants to poke at the SIL Parser (a part of the code base that has not gotten as much attention as others).

Michael

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev


(Michael Gottesman) #10

I feel that the SILParser crasher one is too open ended for a specific bug report. But I would suggest looking into those as well. Here are some more concrete things:

https://bugs.swift.org/browse/SR-339
https://bugs.swift.org/browse/SR-340
https://bugs.swift.org/browse/SR-341
https://bugs.swift.org/browse/SR-342

···

On Dec 22, 2015, at 1:48 PM, Michael Gottesman via swift-dev <swift-dev@swift.org> wrote:

You know what, let me file some bugs. I just thought of some more.

On Dec 22, 2015, at 1:45 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

Here are some off the top of my head. Many are related to code quality. Tell me what you think:

1. We have recently gotten some SILParser crashers. These need to be fixed. I looked at 1-2 of them and the ones I looked at are where we should be giving out a diagnostic but are instead just asserting.

2. The SILParser is a recursive descent parser. In general when one implements a recursive descent parser, one should above the routine that does the various parts of the parsing show the grammar being parsed. SILParser has inconsistently done this. I would go through all of the methods and see which are missing this documentation and fix the documentation.

3. parseSILInstruction is horrible and makes me cry every time I see it. It is a method that is ~1900 lines with a huge switch in it. We should refactor it into a visitor structure. In fact it is large enough that we should consider moving it into its own file if it is possible.

Michael

On Dec 22, 2015, at 12:02 PM, Davide Italiano <dccitaliano@gmail.com <mailto:dccitaliano@gmail.com>> wrote:

On Sun, Dec 13, 2015 at 4:29 PM, Michael Gottesman via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
This is a small starter project for those who are interested in working with SIL.

The SIL Parser currently is unable to parse switch_enum of undef. I wrote a patch that does the work some time ago, but I never have had time to finish it (i.e. make sure everything works ok/write tests). I posted the patch in this issue:

https://bugs.swift.org/browse/SR-210

My hope is that even though a lot of the work is already done this may serve as good starting point for someone who wants to poke at the SIL Parser (a part of the code base that has not gotten as much attention as others).

Michael

Hi Michael,
I'm interested in something like this. I noticed this project has been already taken, but you mentioned in a subsequent mail you have a list of them. What are the other proposals?

Thank you,

--
Davide

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev