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

Label defs and env should not be tied to the main env #77

Open
TravisCarlson opened this issue Oct 16, 2017 · 8 comments
Open

Label defs and env should not be tied to the main env #77

TravisCarlson opened this issue Oct 16, 2017 · 8 comments

Comments

@TravisCarlson
Copy link
Contributor

I get the following error when attempting to forward to a labelStmt.

error: INTERNAL compiler error: expected to find label in function scope, was missing.

The remaining use of txtStmt in the Cilk extension is related to this. Though I've also tried on a simpler extension and found the same error.

https://github.com/melt-umn/ableC-cilk/blob/develop/grammars/edu.umn.cs.melt.exts.ableC.cilk/abstractsyntax/Sync.sv#L103

txtStmt("_cilk_sync" ++ toString(syncCount) ++ ":;")
-- TODO: replace txtStmt with labelStmt
-- labelStmt(name("_cilk_sync" ++ toString(syncCount), location=builtinLoc(MODULE_NAME)), nullStmt())
@krame505
Copy link
Member

Seems to be an issue with functiondefs not being passed correctly, either by the host or by the extension. How do I reproduce this?

@krame505
Copy link
Member

After taking a look, the problem is the interfering override of functiondefs by cilk_syncStmt. This isn't really possible to avoid, since computing env at this point requires the value of functiondefs (FYI, globalDecls, defs, and freeVariables do not affect the incoming environment to a statement, so those equations are redundant.)

This issue really stems from our combining of all environment namespaces into one inherited attribute, and the resulting flow constraints. The forward really doesn't depend on the label environment, only on the misc namespace in this case. You could think of other cases where we would want to only depend on the value namespace, or something else; we essentially would like to be able to access one environment namespace without demanding all the other namespaces be available.

Fortunately, the major namespaces for values, tags, and refIds seem to be somewhat interconnected and either will be all present or all absent in most situations. Extension-added namespaces are also somewhat constrained to be the same anyway, since these are really only useful in determining a forward, or other analyses that depend on the forward. IDK about the misc namespace - maybe this is OK I guess?

So the solution for this seems to be to split off labels into their own environment attributes - labels are kinda special anyway, since they are visible at locations before their declaration, which is the only reason why we need functiondefs in the first place. We could just have a seperate labelEnv inherited attribute and a corresponding labelDefs synthesized attribute that replaces functiondefs entirely. These probably don't even need their own nonterminals, and could just have types Scope<LabelItem> and Contribs<LabelItem>, respectively. If any other namespaces need to split off the main env or are introduced, they could follow a similar pattern.

@tedinski any thoughts on this?

@TravisCarlson
Copy link
Contributor Author

I can reproduce this same error on extensions that don't override functiondefs. For example, if I change the forwarding equation of applyExpr in the closure extension to:

  forwards to
    stmtExpr(labelStmt(name("label" ++ toString(genInt()), location=bogusLoc()), nullStmt()),
      mkErrorCheck(localErrors, fwrd), location=bogusLoc());

https://github.com/melt-umn/ableC-closure/blob/develop/grammars/edu.umn.cs.melt.exts.ableC.closure/abstractsyntax/Apply.sv#L41

@TravisCarlson
Copy link
Contributor Author

Looks like that second example was related to the labelStmt being within a stmtExpr, and functiondefs doesn't occur on Expr. This means we can't do something like the example below, which gcc accepts, but is kind of pointless because gcc doesn't allow a goto to it. But this is different than what the Cilk extension is trying to do.

int main(void) 
{   
    ({l1: ; 0;});
    return 0;
}

@krame505
Copy link
Member

Yeah, the fix for this (a bit contrived) case would be to make functiondefs occur on Expr. But the problem with the cilk extension (and the halide extension, btw, but we aren't actually using labels there) is that there is no way around overriding functiondefs.

@tedinski
Copy link
Member

I haven't looked closely at this, but do labels inside stmt-exprs really propogate up to the top-level function?

I have no idea what jumping to one would mean!

@krame505
Copy link
Member

It appears GCC raises an error with a direct goto into a statement-expression, but they certainly do propagate, as I can do a computed goto!

int main(void) 
{   
    ({l1: ; 0;});
    void *ptr = &&l1;
    goto *ptr;
    return 0;
}

Anyway, this is a lower-priority problem, for which I will open a seperate issue. The real blocking issue here is the circular dependency with the env as described above.

@krame505 krame505 changed the title labelStmt error Label defs and env should not be tied to the main env Oct 16, 2017
@krame505
Copy link
Member

I am currently working on fixing this in the feature/env branch, amongst some other random env refactoring.

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

No branches or pull requests

3 participants