Minres / CoreDSL

Xtext project to parse CoreDSL files
Apache License 2.0
16 stars 3 forks source link

Basic Analysis Framework + Elaboration #59

Closed AtomCrafty closed 2 years ago

AtomCrafty commented 2 years ago

This PR marks the start of a new analysis implementation. It lives in com.minres.coredsl.analysis and my plan is to eventually replace all of the existing analysis facilities, including the TypeProvider and CoreDslInterpreter.

Even though it is still a draft, the elaboration is mostly functional and can be tested by running the IDE project.

Architecture

All of the analyzers are static classes, meaning they cannot be instantiated and provide their functionality through static methods. Because of this they cannot store any state, which is why they are passed an AnalysisContext or ElaborationContext for every operation.

Analysis Context

Full path: com.minres.coredsl.analysis.AnalysisContext Replaces: nothing

Reponsibilities:

Elaboration Context

Full path: com.minres.coredsl.analysis.ElaborationContext Replaces: com.minres.coredsl.interpreter.EvaluationContext

Reponsibilities:

The difference between the analysis context and the elaboration context is that the elaboration context is specific to each elaboration root ISA. Whenever logic inside of an ISA is analyzed, the elaboration context is needed to determine which core definition (or instruction set in case of partial elaboration) is being elaborated. For static analysis that is only performed once instead of for each core, the analysis context is used.

Type Classes

Full path: com.minres.coredsl.type.* Replaces: com.minres.coredsl.typing.DataType

Reponsibilities:

Constant Value Class

Full path: com.minres.coredsl.analysis.ConstantValue Replaces: com.minres.coredsl.interpreter.Value

Reponsibilities:

Type Provider

Full path: com.minres.coredsl.analysis.CoreDslTypeProvider Replaces: com.minres.coredsl.typing.TypeProvider

Reponsibilities:

Important methods:

Related classes:

Constant Expression Evaluator

Full path: com.minres.coredsl.analysis.CoreDslConstantExpressionEvaluator Replaces: com.minres.coredsl.interpreter.CoreDslInterpreter

Reponsibilities:

Important methods:

Related classes:

Elaborator

Full path: com.minres.coredsl.analysis.CoreDslElaborator Replaces: nothing

Reponsibilities:

Important methods:

Related classes:

Analyzer

Full path: com.minres.coredsl.analysis.CoreDslAnalyzer Replaces: nothing

Reponsibilities:

AtomCrafty commented 2 years ago

@jopperm @eyck

The remaining failing test cases all depend on this file: https://github.com/Minres/RISCV_ISA_CoreDSL/blob/master/RISCVBase.core_desc

The issue is that it declares several aliases, which are not marked as register or extern. All ISA state declarations without an explicit storage class currently default to the storage class param and ISA parameters may not be declared as an alias or array.

I see two ways of handling this issue: Enforcing an explicit storage class specifier or making all alias declarations default to a different storage class. I prefer the first approach. The second one is flawed, because alias-ness is a property of the declarator, while the storage class is a property of the declaration as a whole. So a declaration with both an alias and a non-alias declarator would no longer have a well-defined storage class:

unsigned int x = 5, &alias = PC;
eyck commented 2 years ago

Well, an alias itself does not have a storage class as it is another name for a variable and as such inherits the storage class. It is an error if the type of an alias is not the same as the original variable.

jopperm commented 2 years ago

Hmm, the alias thing is interesting. My first hunch also is that we shouldn't have to repeat the storage class on the alias. An alias is a declaration with an ampersand in it, so that should suffice to mark it as an alias, and in consequence, inherit the storage class from the entity it points to.

However, the same reasoning could then also be applied to the type specifier. On the other hand, I wanted to make it possible to change the constness and volatileness of an alias (e.g. to have a read-only alias to some writable entity).

AtomCrafty commented 2 years ago

I guess we could sidestep the alias issue by not allowing mixed declarations. Then we could introduce a new alias storage class and in case no explicit storage class is specified, we default to either param or alias depending on the declarators:

int a, b;           // implicit storage class "param"
int &x = a, &y = b; // implicit storage class "alias"
int p, &q = b;      // error, mixed declaration
jopperm commented 2 years ago

I guess we could sidestep the alias issue by not allowing mixed declarations. Then we could introduce a new alias storage class and in case no explicit storage class is specified, we default to either param or alias depending on the declarators:

int a, b;           // implicit storage class "param"
int &x = a, &y = b; // implicit storage class "alias"
int p, &q = b;      // error, mixed declaration

Yes, that SGTM! (minus adding an alias keyword, cf. the thread above)

AtomCrafty commented 2 years ago

Ready for review. @jopperm Could you please add the table from https://github.com/Minres/CoreDSL/pull/59#discussion_r973708842 or a variation thereof to the spec?

Also I have no clue why the CI is failing. All tests are successful on my end. The errors reported by the pipeline are complete nonsense and refer to line numbers that don't even exist.

AtomCrafty commented 2 years ago

The ProxyMessageAcceptor class is used to automatically report errors in the correct place. If during elaboration of a core, an issue is reported within a provided instruction set, the proxy acceptor will report that issue on the core's name token. It also prevents us from accidentally reporting issues in other source files, which would cause the validator to crash.

eyck commented 2 years ago

Well, the gradle test reports about the Java lines but you are looking at the xtend sources therefore they don't match.

jopperm commented 2 years ago

@jopperm Could you please add the table from #59 (comment) or a variation thereof to the spec?

Will do!

jopperm commented 2 years ago

Done: diff

jopperm commented 2 years ago

I also made the change that only parameter declaration are merged: diff