Unused code in visitors

[asl]

There are some bits in visitors that are not used and not correspondingly lacking any test coverage.

Examples are:

• dontForwardChildrenBeforePreorder
• loop_revisit and corresponding boilerplate

Are they stale? Or used downstream? Can they be just removed?

Tagging @grg @ChrisDodd @vlstill @fruffy

[grg]

Both of these are used by the Tofino compiler, so please don't remove 🙂

[asl]

@grg Thanks!

Do you use visitAgain / visitOnce?

[grg]

Do you use visitAgain / visitOnce?

Yes, we use both.

[fruffy]

Any chance some of the code using it can be upstreamed? Would make it clearer how to use it.

[grg]

Any chance some of the code using it can be upstreamed? Would make it clearer how to use it.

Nothing obvious that makes sense to upstream: a lot of the code is in the backend. I'm going to see if I can at least extract out the core ideas and put them in unit tests.

[ChrisDodd]

loop_revisit and corresponding boilerplate

This is actually still work in progress, to allow some ability to deal with IR loops in visitors. But as it is, it is very useful for, eg, visiting the definition of a symbol recursively (in the same visitor) whenever you visit a reference to that symbol. You can do stuff like:

SomePass::preorder(const IR::PathExpression *pe) {
    if (auto *def = reolveUnique(pe->path.name, ResolutionType::Any)) {
        const IR::Node *node = def->getNode();   // FIXME -- still can't visit a INode directly :-(
        visit(node);

If you have mutually recusive definitions (a refers to b and b refers to a) then this will end up hitting loop_revisit and you must resolve it somehow (or issue an error message if your hardware can't deal with it.

There's a much improved midend def-use pass in Intel's code that I had intended to "promote" to open source, but never got around to it before I left Intel :-(

I also have aspirations to extend ControlFlowVisitor to deal with loops by repeatedly visiting things until a fixed-point is reached. Needs a change in the flow_merge function to return a flag that tells if a fixed point has been reached or not.

[vlstill]

dontForwardChildrenBeforePreorder

I've come around a use case for that recently too. When IR is a DAG it is sometimes useful to see the old values in a transform, especially when doing things like IR lowering to a backend dialect. I guess constants, types and paths could be some examples of node types that may occur in multiple locations in the IR.

We might want to comment some use cases for these things which are there for downstream tools' benefit.

[asl]

We might want to comment some use cases for these things which are there for downstream tools' benefit.

Yeah. It would be great to have some test coverage as some things look like to be unused or used only once, etc. Also, maybe these options should be set in constructor and not on-fly.

[fruffy]

Reopening because there might be some things to do here.

There's a much improved midend def-use pass in Intel's code that I had intended to "promote" to open source, but never got around to it before I left Intel :-(

How difficult is this to do? Or what are the improvements?

[grg]

There's a much improved midend def-use pass in Intel's code that I had intended to "promote" to open source, but never got around to it before I left Intel :-(

I was wondering the other day whether I could upstream that pass. It would be a useful pass to have access to, and I don't think there's too strong a reason to keep it private to Intel.

[asl]

There's a much improved midend def-use pass in Intel's code that I had intended to "promote" to open source, but never got around to it before I left Intel :-(

I was wondering the other day whether I could upstream that pass. It would be a useful pass to have access to, and I don't think there's too strong a reason to keep it private to Intel.

This might be a useful addition. The present use-def pass is quite... slow and memory hungry (and this is part of slowdown of P4CParserUnroll.switch_20160512 testcase). I am having some fixes to it. But if there is better solution, then why not.

[grg]

There's a much improved midend def-use pass in Intel's code that I had intended to "promote" to open source, but never got around to it before I left Intel :-(

I was wondering the other day whether I could upstream that pass. It would be a useful pass to have access to, and I don't think there's too strong a reason to keep it private to Intel.

This might be a useful addition. The present use-def pass is quite... slow and memory hungry (and this is part of slowdown of P4CParserUnroll.switch_20160512 testcase). I am having some fixes to it. But if there is better solution, then why not.

I'll put up a PR this week with the midend def-use pass.