Remove Legacy Rewrites

[Jesper Öqvist]

This is a proposal to remove the legacy rewrite mechanism.

The circular NTA implementation of rewrites has been used without problems for a long time now in ExtendJ.

CNTAs are much simpler to understand than legacy rewrites and they have much smaller implementation. CNTA rewrites are implemented as implicit circular attributes and almost no specialized code generation is needed. In contrast, the legacy rewrite mechanism has lots of conditional template parts that can be removed when the feature is killed.

By removing legacy rewrites we will also finally kill list rewrites (see issue #141).

The next release without legacy rewrites should be a major release (2.4.0).

[Jesper Öqvist]

I pushed a branch that removes legacy rewrites, called rewriteectomy. The corresponding branch on jastadd-test has updated the tests that depend on legacy rewrites.

[Jesper Mattsson]

Does this mean that you can remove the behavior that List.rewriteTo() triggers rewrites for all children if any child has been added? That would mean that you no longer get O(n^2) for the code:

A a = new A();
for (...) {
    a.addB(new B(...));
}

[Jesper Öqvist]

@jespermattsson The new (CNTA) rewrite implementation fixes that problem. However, it is still possible to have a rewrite on node A and then adding new nodes to A is only safe if you have not yet accessed A via the parent tree.

Since CNTAs are just regular attributes, they are evaluated on demand. With CNTAs eager rewriting only happens if you clone the tree with a treeCopy() (but not with treeCopyNoTransform).

[Jesper Mattsson]

@joqvist I'm not sure what you are getting at? The snippet above would typically be in a method that builds part of the tree, possibly the parser (although there it wouldn't be a for loop).

I guess my question boils down to: Will the following code be removed from List.rewiteTo() as a part of this issue?

        if (list$touched) {
            for(int i = 0 ; i < getNumChildNoTransform(); i++) {
                getChild(i);
            }
            list$touched = false;
            return this;
        }

[Jesper Öqvist]

@jespermattsson list$touched is not part of the CNTA implementation and will be removed.

I did not notice that you created a new A node in your example, which is totally safe. What I meant with the previous comment was that this is not safe:

A a = someTree.getA();
for (...) {
    a.addB(new B(...));
}

[Jesper Mattsson]

Great!

Yes, that snippet looks like it belongs in the "at you own risk" bin.

[Jesper Öqvist]

@jespermattsson I tried running OptimicaCompiler (JModelica 2.2b5) tests with the new rewrite mechanism. Here are things I needed to change to make it compile:

1. JModelica manually changes the is$Final flag in a few spots to suspend rewriting. This does not work with the new rewrite system and there is no way to disable rewrites.

2. In one case, a _computed value is changed to recompute an attribute. This broke because the _computed flag is no longer a boolean value (it is a circular evaluation ID). This was easy to fix and the change should preserve the old behavior:

--- Compiler/ModelicaFrontEnd/src/jastadd/errorcheck/ContentsCheck.jadd (revision 10917)
+++ Compiler/ModelicaFrontEnd/src/jastadd/errorcheck/ContentsCheck.jadd (working copy)
@@ -868,7 +868,7 @@


     public void InstComponentDecl.setVariabilityNotCalculated() {
-        if (!variability_computed) {
+        if (variability_computed == null) {
             return;
         }
         InstComponentDecl icd = enclosingInstComponentDecl();
@@ -875,11 +875,11 @@
         if (icd != null) {
             icd.setVariabilityNotCalculated();
         }
-        variability_computed = false;
+        variability_computed = null;
     }

     public void FAbstractExp.setVariabilityNotCalculated() {
-        variability_computed = false;
+        variability_computed = null;
     }

 }

The tests failed after applying these changes - testing hangs somewhere. My current hypothesis is that the removal of the is$Final flag causes the testing to get stuck: JModelica relied on suspending rewrites and without that the evaluation gets stuck in an infinite rewrite loop somewhere.

Since the is$Final flag disappeared, it might be tempting to add a custom rewrite flag. However, it will not work. A custom rewrite flag is added for example in Compiler/ModelicaFrontEnd/src/jastadd/flattening/Connections.jrag:

    public void FStreamBuiltIn.enableStreamsRewrite() {
        super.enableStreamsRewrite();
        rewriteStreams = true;
    }

    protected boolean FStreamBuiltIn.rewriteStreams = false;

    /**
     * Rewrite the inSteam operator to expanded form.
     */
    rewrite FInStream {
        when (rewriteStreams) to FExp expandInStreamExp();
    }

Custom rewrite flags do not work because JastAdd does not want to reevaluate the rewrite condition for each child access, so it trusts that if it already rewrote the node once it should not change (the rewrite already did a fixpoint iteration to reach the final state).

It could be possible to add the is$Final flag to the new rewrite implementation to check if the Optimica tests pass with it, but I'm not sure it is a feature that should exist in JastAdd.

[Jesper Öqvist]

I tried adding the is$Final flag to my work-in-progress rewrite removal branch. It did not help. The tests still hang with/without --safeLazy.

[Jesper Mattsson]

The pattern you are referring to is where we want to activate rewrites after a certain step in the processing of the flat tree. We set a flag on the nodes that are to be rewritten, and then flush all cached values for the entire tree and clear the is$Final flag to enable the rewrite. I guess we'll just have to rewrite that code to use the same mechanisms for changing the tree as we usually do, i.e. explicitly changing it with setX(), then flushing cached attributes.

The alternative of changing everything we do in the flat tree to use JastAdd paradigms isn't feasible, I'd say.

[Jesper Öqvist]

Removed legacy rewrite mechanism and list rewrites

• Rewrites are now always generated using the circular NTA mapping.
• The safeLazy option is now enabled by default.

fixes #141 (bitbucket) fixes #293 (bitbucket)

→ <<cset 124b0bc0f4a2>>

[Jesper Öqvist]

Removed legacy rewrite mechanism and list rewrites

• Rewrites are now always generated using the circular NTA mapping.
• The safeLazy option is now enabled by default.

fixes #141 (bitbucket) fixes #293 (bitbucket)

→ <<cset 7ade56d70d44>>