Page MenuHomePhabricator

[TableGen] Introduce an if/then/else statement.
ClosedPublic

Authored by simon_tatham on Dec 13 2019, 5:26 PM.

Details

Summary

This allows you to make some of the defs in a multiclass or foreach
conditional on an expression computed from the parameters or iteration
variables.

It was already possible to simulate an if statement using a foreach
with a dummy iteration variable and a list constructed using !if so
that it had length 0 or 1 depending on the condition, e.g.

foreach unusedIterationVar = !if(condition, [1], []<int>) in { ... }

But this syntax is nicer to read, and also more convenient because it
allows an else clause.

To avoid upheaval in the implementation, I've implemented if as pure
syntactic sugar on the foreach implementation: internally, ParseIf
actually does construct exactly the kind of foreach shown above (and
another reversed one for the else clause if present).

Diff Detail

Event Timeline

simon_tatham created this revision.Dec 13 2019, 5:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2019, 5:26 PM

This is a hasty first draft because I'm about to go away for the holidays, and won't have access to this account until the New Year.

It certainly needs documentation, and might also benefit from a refactoring of the mechanism (perhaps the stack of ForeachLoops ought to become a stack of some kind of polymorphic type that can be either a loop or an if).

This patch applies on top of the defvar one (D71407), and in this version, I've made it so that unlike a foreach, an if statement doesn't begin a new local-variable scope. I think that might be useful because that way you can conditionally define the same local var in both branches of the if, and then use it afterwards.

This is a hasty first draft because I'm about to go away for the holidays, and won't have access to this account until the New Year.

This looks like a useful feature.

It certainly needs documentation, and might also benefit from a refactoring of the mechanism (perhaps the stack of ForeachLoops ought to become a stack of some kind of polymorphic type that can be either a loop or an if).

This patch applies on top of the defvar one (D71407), and in this version, I've made it so that unlike a foreach, an if statement doesn't begin a new local-variable scope. I think that might be useful because that way you can conditionally define the same local var in both branches of the if, and then use it afterwards.

I see the use case, but I'm not sure that it's worth breaking the regularity of the scoping scheme.

I agree with Hal that if/else should introduce a scope. Otherwise, you get into consistency problems. Consider:

multiclass Foo<...> {
   if ... then {
      defvar bar = ...;
   }
   defvar baz = bar;
}

Without introducing a scope, the above is presumably valid but in a rather surprising way. And a similar example with an else that also defines bar wouldn't be valid due to the redefinition.

Generally I'm in favor of this feature, and desugaring it as a loop is a good idea as it should then compose nicely with other features. Some more tests are needed, in particular its use in a multiclass and (negative test) in a def and class.

Without introducing a scope, the above is presumably valid but in a rather surprising way.

Er, wow, yes – when I tried it, it surprised me!

I had expected that if the then clause wasn't taken then a definition of bar from outside the multiclass would be used in place of the skipped definition (or an error would happen if there wasn't one). But in fact the definition wasn't skipped at all, which I agree is more surprising even than I expected. So no wonder it also doesn't work to define the same variable in both then and else branches (which I intended to work, but you're right that it currently doesn't).

Luckily I don't have to solve that mystery, since both of you agree that the behavior I intended was the wrong behavior anyway, and the one you're asking for is much easier to get right :-)

Added documentation and more tests; changed scoping policy so that then and else clauses do make a new scope for defvar; rebased on top of the revised D71407.

@nhaehnle, @hfinkel: anything left I need to do to this?

hfinkel accepted this revision.Jan 14 2020, 4:03 AM

@nhaehnle, @hfinkel: anything left I need to do to this?

LGTM

This revision is now accepted and ready to land.Jan 14 2020, 4:03 AM
This revision was automatically updated to reflect the committed changes.