[do not merge]: replace typeMap->constants with IR-based constants #526
Conversation
Defines a member in the IRCompileTimeValue class to determine whether the node is a compile time constant.
…ants This commit eliminates the use of the typeMap->constants to keep track of compile time constants, and relies on the IR nodes to know whether they are constants themselves. There are a couple of instances (Members and PathExpressions) where the compile time constantness is defined by the type declaration. In this case we add a method just to those nodes to give them the ability to be set as constants.
There was a problem hiding this comment.
The information about whether something is a compile-time constant is only used during type-checking. So I think that storing it in the IR is overkill. You just need to compute a set<IR::Node> for all nodes that are compile-time constants. This information is used to reject programs that are malformed and is no longer used afterwards. This probably can be factored into an Inspector pass.
BTW: this is the whole point of the visitor pattern: you (almost never) need to change the IR.
|
Actually it should be used in constantFolding, no? |
|
No, constant folding maps nodes to actual values. SideEffects does not need this, it just updates the information so it remains correct. |
|
Leaving aside the issue of whether we should do this, this came up again today and I thought it might be worth recording here that "isLeftValue()" is something else we could store directly on the nodes instead of in a table if we so chose. |
|
`isLeftValue` seems to be redundant with `P4WriteContext`, which tells you
whether an expression is used an an lvalue or not. Storing it on/in a node
or in an external table indexed by node is impossible, as a node may be
referenced in mulitple places, some of which are lvalues and some of which
are not. This is a fundamental problem with the typeMap approach and leads
to issues with dags (which is why some other passes make redundant/useless
clones -- to work around the limitations of the typeMap).
…On Tue, Jun 20, 2017 at 7:48 PM, Seth Fowler ***@***.***> wrote:
Leaving aside the issue of *whether* we should do this, this came up
again today and I thought it might be worth recording here that
"isLeftValue()" is something else we could store directly on the nodes
instead of in a table if we so chose.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#526 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AD4c8bwV7T7joh6UPzkHcJEAKunHPQ13ks5sGISJgaJpZM4NIYAU>
.
|
|
@cc10512 : are you still interested to refactor this piece of code? |
|
Since this is 3 years old I will close it. |
This PR attempts to replace the typeMap->isCompileTimeConstant/setCompileTimeConstant with that functionality handled by IR nodes.
Note that less than half of all tests pass with this change. Most fail with the dreaded:
@mbudiu-vmw: I need help in figuring out which of the IR nodes in typeChecker is missing the right information. I traced all the changes and they seem to capture all the places where constants were set. For one of the test cases (testdata/p4_16_samples/inline-stack-bmv2.p4), I tracked this down to invoking the apply method on the aux instance in the sequence below. Commenting out the aux.apply line allows the program to go through.