Skip to content

[WIP] Eliminate branch when the condition is false#3133

Closed
baziotis wants to merge 1 commit intoldc-developers:masterfrom
baziotis:if_false
Closed

[WIP] Eliminate branch when the condition is false#3133
baziotis wants to merge 1 commit intoldc-developers:masterfrom
baziotis:if_false

Conversation

@baziotis
Copy link
Contributor

Partially solving (i.e. only false conditions): #2344
It does not work! You can even see that 2 test cases I added fail. Now that I saw the issue again, I should add a test case for labels.
I'm uploading just for opinions. Essentially the problems are:

  • Doesn't test for loops. Probably every statement (if, for, while - probably there are others I can't think now) should be handled separately.
  • The false_cond_with_else.d should be easy to pass. Fortunately, the front-end breaks an else if (cond) to else { if (cond) } so if there is an else body, that should be handled.
  • The label thing is the one I'm not sure about. Check all the statements in the body for a label statement ? I don't know.. I'll come back to it.

@JohanEngelen
Copy link
Member

Some comments:

  • focus on if(0) in this PR, do all the other things later
  • I would omit the constant propagation testing in the tests here
  • for labels, indeed I think the only thing you can do is search all statements for labels... This is painful. Perhaps you can first work on adding that functionality to the semantic analysis in the frontend. It would probably be cheap to store a list all labels in a function during semantic analysis because you are already iterating over all statements. Discuss it with the frontend people, I think it may make sense for other functionality aswell to have this information more readily available.

@JohanEngelen
Copy link
Member

Also: try and find out how DMD, GDC, (and Clang) tackle this problem.

@baziotis
Copy link
Contributor Author

baziotis commented Aug 15, 2019

Some comments:

  • focus on if(0) in this PR, do all the other things later
  • I would omit the constant propagation testing in the tests here
  • for labels, indeed I think the only thing you can do is search all statements for labels... This is painful. Perhaps you can first work on adding that functionality to the semantic analysis in the frontend. It would probably be cheap to store a list all labels in a function during semantic analysis because you are already iterating over all statements. Discuss it with the frontend people, I think it may make sense for other functionality aswell to have this information more readily available.

Great thanks! I think it's better to close this PR and open a new more specific one. I'll ask on slack if we have any info for labels and how difficult would be for me to add it.

Edit: Btw now that I'm thinking of that, getting a list of all the labels in a function is not enough. There has to be some location info about each label and I'm not yet sure what this info can look like. Or even if the labels per se are the point of interest here.
For example, let's take the "simple" elimination where if an if body has a label anywhere inside, then generate the body (A more complicated elimination would be to still not generate it if there's no goto to this label but let's skip that). Then, what we need is something like "hasLabel".
Unless you had something else in mind and I missed it.

@baziotis
Copy link
Contributor Author

Also: try and find out how DMD, GDC, (and Clang) tackle this problem.

I believe this is a very good advice. Unfortunately, and these are only things I've heard not that I have worked with any of the two: the DMD back-end is one of the most obscure (i.e. pretty much only Walter understands it). The GDC front-end too (i.e. only Iain understands it). I'll have a look at them, but I dare to say that maybe the most approachable is Clang.

@baziotis
Copy link
Contributor Author

Continues here: #3134

@baziotis baziotis closed this Aug 15, 2019
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