Page MenuHomePhabricator

[TableGen] Introduce a `defvar` statement.
ClosedPublic

Authored by simon_tatham on Dec 12 2019, 3:22 AM.

Details

Summary

This allows you to define a global or local variable to an arbitrary
value, and refer to it in subsequent definitions.

The main use I anticipate for this is if you have to compute some
difficult function of the parameters of a multiclass, and then use it
many times. For example:

multiclass Foo<int i, string s> {
  defvar op = !cast<BaseClass>("whatnot_" # s # "_" # i);
  def myRecord {
    dag a = (op this, (op that, the other), (op x, y, z));
    int b = op.subfield;
  }
  def myOtherRecord<"template params including", op>;
}

There are a couple of ways to do this already, but they're not really
satisfactory. You can replace defvar x = y with a loop over a
singleton list, foreach x = [y] in { ... } - but that's unintuitive
to someone who hasn't seen that workaround idiom before, and requires
an extra pair of braces that you often didn't really want. Or you can
define a nested pair of multiclasses, with the inner one taking x as
a template parameter, and the outer one instantiating it just once
with the desired value of x computed from its other parameters - but
that makes it awkward to sequentially compute each value based on the
previous ones. I think defvar makes things considerably easier.

You can also use defvar at the top level, where it inserts globals
into the same map used by defset. That allows you to define global
constants without having to make a dummy record for them to live in:

defvar MAX_BUFSIZE = 512;

// previously:
// def Dummy { int MAX_BUFSIZE = 512; }
// and then refer to Dummy.MAX_BUFSIZE everywhere

Diff Detail

Event Timeline

simon_tatham created this revision.Dec 12 2019, 3:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2019, 3:22 AM

I've been working around the lack of this for a while, so I thought it was worth at least trying to get it accepted.

This is something of a first draft. Things I'm not certain of:

  • should a defvar in a foreach be local to it? (Probably)
  • should I allow defvar inside a class or def as well as in a multiclass, defining variables that vanish at the closing brace but can be referred to by the definitions of proper class member values? (Possibly)
  • is defvar the sensible name? (def and let were already taken; I considered C++ using, but thought defvar fits reasonably well with the existing defset)

I've been working around the lack of this for a while, so I thought it was worth at least trying to get it accepted.

I think that this is a good idea. We spend a lot of time programming in TableGen, and we should do ourselves the favor of giving it reasonable programming-language features, and I think that local variables seem like just the kind of feature that it would be reasonable to add. This prevents unnecessary repetition of expressions or the unnecessarily-verbose uses of foreach or multiclasses.

This is something of a first draft. Things I'm not certain of:

  • should a defvar in a foreach be local to it? (Probably)

I agree. This seems like the consistent behavior.

  • should I allow defvar inside a class or def as well as in a multiclass, defining variables that vanish at the closing brace but can be referred to by the definitions of proper class member values? (Possibly)

This also seems consistent with the feature overall. Are there reasons why we might not want to do this?

  • is defvar the sensible name? (def and let were already taken; I considered C++ using, but thought defvar fits reasonably well with the existing defset)

Or just var? defvar seems fine to me, and seems consistent with other TableGen syntax.

This also seems consistent with the feature overall. Are there reasons why we might not want to do this?

I can't think of a reason it's a bad idea. It's less vital in a class because you can define variables anyway (if you don't mind them showing up in the eventual records under some throwaway names you never look at). But yes, I think as long as I'm doing this anyway, I might as well make it work everywhere it might be needed, to avoid confusion.

We spend a lot of time programming in TableGen, and we should do ourselves the favor of giving it reasonable programming-language features

Another thing on my wish list is an if statement, which you could use in (say) a multiclass or foreach to make one or more entire defs conditional on some function of the current parameters. Similar to defvar, that's a thing you can already fake up with a weird-looking foreach:

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

but it would be shorter and clearer not to have to! (Plus it could have an else clause.)

Revised patch which keeps local-variable scopes in a stack, and opens
a temporary scope for each multiclass body, object body, or foreach body.

This is now out of 'first draft' status and ready for proper review.

hfinkel added inline comments.Dec 13 2019, 5:06 PM
llvm/docs/TableGen/LangRef.rst
430

or class/record local

or should we just say a "scope-local variable" (including some kind of global scope)

  • should a defvar in a foreach be local to it? (Probably)
  • should I allow defvar inside a class or def as well as in a multiclass, defining variables that vanish at the closing brace but can be referred to by the definitions of proper class member values? (Possibly)
  • is defvar the sensible name? (def and let were already taken; I considered C++ using, but thought defvar fits reasonably well with the existing defset)

This is a good feature, and I'd say yes to all three for consistency. Thank you for working on it! The overall design of the code looks reasonable to me, though I do have some nitpicks.

llvm/lib/TableGen/TGParser.h
79–80

Why the inconsistent capitalization? I'm fine with using lower case since there seems to be rough consensus on going there eventually.

83

The initializer is unnecessary.

88–90

This feels unidiomatic to me.

95–100

Just make this a sequence of if()s (no else).

144

The initializer is unnecessary.

simon_tatham marked 8 inline comments as done.Jan 2 2020, 4:54 AM
simon_tatham added inline comments.
llvm/docs/TableGen/LangRef.rst
430

Yes – I didn't revisit these docs after rethinking the scoping system. I've rewritten the whole section to try to be more specific about what does and doesn't work.

llvm/lib/TableGen/TGParser.h
88–90

Hmm. I did that because I wasn't confident that doing a std::move out of a unique_ptr would leave it in a well defined null-pointer state – my reading of the C++ standard is that generally a moved-from library object is in a state where you can safely destruct or assign over it but otherwise unspecified, and unique_ptr isn't an exception as far as I know.

But looking again, of course the point of extractParent is precisely that we're about to destruct this object and its contained unique_ptr, so there's no reason I can't just std::move directly out of parent in the more obvious way, and not worry about what state it's left in.

simon_tatham marked 2 inline comments as done.

Addressed all review comments (I think).

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

hfinkel accepted this revision.Jan 13 2020, 8:00 PM

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

LGTM

This revision is now accepted and ready to land.Jan 13 2020, 8:00 PM
This revision was automatically updated to reflect the committed changes.