Page MenuHomePhabricator

TableGen: Allow foreach in multiclass to depend on template args
ClosedPublic

Authored by nhaehnle on May 27 2018, 1:48 PM.

Details

Summary

This also allows inner foreach loops to have a list that depends on
the iteration variable of an outer foreach loop. The test cases show
some very simple examples of how this can be used.

This was perhaps the last remaining major non-orthogonality in the
TableGen frontend.

Change-Id: I79b92d41a5c0e7c03cc8af4000c5e1bda5ef464d

Diff Detail

Repository
rL LLVM

Event Timeline

nhaehnle created this revision.May 27 2018, 1:48 PM
simon_tatham added inline comments.Jun 4 2018, 8:58 AM
test/TableGen/foreach-multiclass.td
20 ↗(On Diff #148767)

Hmm, this is generating -print-records output that's not legal tablegen input – the minus sign isn't a valid character in identifier names as far as the lexer's concerned. Is that a problem? I think in this situation I'd have expected a complaint from tablegen ("this concatenation would generate an illegal record name"), rather than silently generating this kind of unexpected output which I could imagine causing knock-on confusion in an unwary backend.

simon_tatham added inline comments.Jun 4 2018, 9:18 AM
lib/TableGen/TGParser.cpp
2917 ↗(On Diff #148767)

While you're looking at this code, should this be tracking whether the DefmName you're adding to Substs here was explicitly given by the user, or made up anonymously on line 2861?

I feel as if the end-product record that comes out of the whole chain of defms and foreaches ought to have its 'anonymous' flag set if any defm or def on the way to it was anonymous, because in that case some part of the final name will include something TableGen made up.

nhaehnle added inline comments.Jun 4 2018, 12:19 PM
lib/TableGen/TGParser.cpp
2917 ↗(On Diff #148767)

I'd agree if we were recreating TableGen from scratch.

However, there are some existing TableGen files (e.g. something in X86 about condition codes) that rely on being able to substitute NAME reliably in a multiclass that is instantiated by a defm without a name. I did ponder changing those TableGen files accordingly while I did the earlier change (about streamlining NAME resolution), but ultimately decided against it to reduce the churn in existing .td files.

While I also think the behavior with the made-up "anonymous_NNNN" part in names is a bit weird, it's a comparatively minor wart that doesn't really hurt anything as far as I can tell.

test/TableGen/foreach-multiclass.td
20 ↗(On Diff #148767)

Fair point. I'll at least change the test case to remove this. I don't know if we can or should actually add such a check.

nhaehnle updated this revision to Diff 149828.Jun 4 2018, 12:23 PM

Address a review comment

simon_tatham accepted this revision.Jun 5 2018, 7:56 AM

In that case, LGTM – though I must admit that this is probably the part of tablegen I understand least well, so I can't guarantee not to have missed anything subtle!

lib/TableGen/TGParser.cpp
2917 ↗(On Diff #148767)

Fair enough -- I suppose if the backend just needs a set of unique string identifiers and doesn't necessarily care exactly what they are, then that's enough.

In that case I should probably revert the tweak in that area I just made to my JSON backend (when I started exporting the anonymous flag). Fortunately you're about to cause a merge conflict that will remind me to do that :-)

This revision is now accepted and ready to land.Jun 5 2018, 7:56 AM
tra accepted this revision.Jun 6 2018, 1:10 PM

Few nits. LGTM overall.

lib/TableGen/TGParser.cpp
392–393 ↗(On Diff #149828)

I think for (auto &E: LI) should work.

1268–1277 ↗(On Diff #149828)

This may warrant a comment explaining why we need a temp record here.
AFAICT, it's supposed to provide a scope for the names used in top-level foreach.

1530–1541 ↗(On Diff #149828)

Same here.

lib/TableGen/TGParser.h
45–48 ↗(On Diff #149828)

Are Rec and Loop expected to be mutually exclusive?
It would be good to have some asserts enforcing the invariant.

test/TableGen/NestedForeach.td
20–22 ↗(On Diff #148767)

Here and below I'd make all nested constructs indented, possibly with braces -- like it's done in the nested foreach above.

nhaehnle marked 10 inline comments as done.Jun 14 2018, 4:33 AM
nhaehnle added inline comments.
lib/TableGen/TGParser.cpp
392–393 ↗(On Diff #149828)

Right you are, changed it (to *LI actually, but that's a detail).

1268–1277 ↗(On Diff #149828)

Right, added a comment here and below.

lib/TableGen/TGParser.h
45–48 ↗(On Diff #149828)

Good point, done.

test/TableGen/NestedForeach.td
20–22 ↗(On Diff #148767)

I'm putting the indent, but keeping it without braces (to make sure that path of the parser is covered as well).

nhaehnle updated this revision to Diff 151329.Jun 14 2018, 4:34 AM
nhaehnle marked 4 inline comments as done.

Address review comments. Going to submit later today.

This revision was automatically updated to reflect the committed changes.