Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

TableGen: Let expressions available to list subscripts and list slices

Authored by chapuni on Mar 12 2023, 7:46 AM.



This enables indexing in !foreach and permutation with list[permlist].

Enhancements in syntax:

  • list<int> is applicable as a slice element.
  • list[int,] is evaluated as not ElemType but list<ElemType> with a single element.

FIXME: I didn't apply new semantics to BitSlice.

TableGen: Prune convertInitListSlice and VarListElementInit

They were dedicated to constant version of list slice.

Depends on D147401

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
chapuni updated this revision to Diff 505516.Mar 15 2023, 8:48 AM
  • Style fixes
chapuni updated this revision to Diff 505820.Mar 16 2023, 8:25 AM
  • Improve error messages
  • Use cast<ty> as possible
chapuni updated this revision to Diff 506893.Mar 21 2023, 3:18 AM
  • Split out ParseSliceElement()
  • Update a test
chapuni updated this revision to Diff 507386.Mar 22 2023, 9:12 AM
  • Rewrite tgtok::IntVal
arsenm added a subscriber: arsenm.Mar 30 2023, 11:32 AM
arsenm added inline comments.
527 ↗(On Diff #507386)

This section is indented further than the others

530 ↗(On Diff #507386)

Typo 'tyep'


This assert seems redundant with the cast<> above

chapuni updated this revision to Diff 509833.Mar 30 2023, 2:59 PM
chapuni marked an inline comment as done.
  • s/tyep/type/
chapuni added inline comments.
527 ↗(On Diff #507386)

It is intentional that this is the subitem as supplement, since I wanted to avoid rewriting whole this item.
Could I rewrote this flat? Or I am happy if you could suggest an alternative sentense.


I intend this as catching RHS = nullptr by default.
Assignments are done in L738 and L752, and I would like to catch the assumption after cases in the switch (in the future).

barannikov88 added inline comments.
348 ↗(On Diff #509833)

IIUC this is supposed to be normative and (negative) is not normative.


What is C in RANGEC? Please give a descriptive name.


I don't get it how IntVal implies -. Could you explain?


Please give entities descriptive / canonical names.


The whole comment should be synced with the documentation.


ParseSimpleValue is not guaranteed to return TypedInit, this should be an error.


It should point to the element, not the opening bracket.

Thanks for reviews! I will fix them later.

348 ↗(On Diff #509833)

I could drop (negative) if you mention the notation.
I know this is less strict but I wanted to explain semantic here.


It is a retronym "RANGE (close interval)" to D145871 (RANGE as half-open interval).
Could I revise both names?


See L754 in TGParser::ParseRangePiece().
Could I update this as // Expects "-num" (deprecated) ?


Will fix. How about Elems and Lists? I don't prefer longer names.


Excuse me, I don't understand the point.


Understood. I will also try finding error cases.


Since I was convinced to look into another user of SquareLoc, I followed it.
Wouldn't it make sense to point SquareLoc as "error in whole range expression"?

barannikov88 resigned from this revision.Mar 30 2023, 8:22 PM
barannikov88 added a subscriber: nhaehnle.

I like the idea, but the implementation looks more like a PoC than a complete solution.
There is clearly an overlap between RangeList and SliceElements and I don't see why they should be distinct.
I am not a TableGen expert so take my words with a grain of salt. I won't be able to review it well.
Maybe @nhaehnle could take a look?

348 ↗(On Diff #509833)

You can update the comment below.


RANGE is fine in D145871 because it directly corresponds to a bang operator.
There is no operator in the language that corresponds to any of LISTELEM/LISTSLICE/RANGEC.

I wish I new TableGen better to suggest a way of properly handling this...
It feels like it would be better to update the deleted code instead of deleting it.


I think it is because 1 - -2 parses the same as 1 2 because --2 == 2.


How about Elems and Lists?

Elems and Lists would be better if it is what they are.


The [old] doc says:

Value: `SimpleValue` `ValueSuffix`*
     :| `Value` "#" [`Value`]
ValueSuffix: "{" `RangeList` "}"
           :| "[" `RangeList` "]"
           :| "." `TokIdentifier`

The [old] comment should have been the same or (in different style):

Value: `SimpleValue` `ValueSuffix`*
Value: `Value` "#" [`Value`]
ValueSuffix: "{" `RangeList` "}"
ValueSuffix: "[" `RangeList` "]"
ValueSuffix: "." `TokIdentifier`

I.e. the current comment misses the paste operator (#) and refers to BitList, which seems to have been changed to RangeList as some point.

Since you are updating the comment it would be nice if it represents the current state of things.


Wouldn't it make sense to point SquareLoc as "error in whole range expression"?

The error message specifically says that LHS has invalid type. Pointing to opening '[' is misleading.
It may make sense (the error message should be changed then), but if you can give a better error location why don't do it?
You don't need to follow old code, it is not always guaranteed to be perfect.

chapuni updated this revision to Diff 510194.Apr 1 2023, 12:44 AM
chapuni marked 3 inline comments as done.
  • TGParser::ParseValue(): Prune one error reporter, since Fold() doesn't return nullptr.
  • llvm/test/TableGen/ Insert whitespaces in tokens
  • Revise symbols in TGParser::ParseSliceElements()
  • TGParser::ParseSimpleValue(): Report LHS position for LHS error.
  • TGParser::ParseValue(): Detect untyped LHS in the list slice
  • ProgRef.rst: Revise to mention also SliceElement.

I like the idea, but the implementation looks more like a PoC than a complete solution.
There is clearly an overlap between RangeList and SliceElements and I don't see why they should be distinct.

In fact I started this as PoC at first but I don't suppose this is still PoC..
When I dropped VarListElementInit, I thought I achieved a step of migrations.
I understand you feel this as PoC, though.

I think the new semantics could be applied also to list initializer.
I haven't done since it didn't prevent my other changes, for list manipulations.
OTOH, I didn't change BitSlice, because I thought it is satisfied with fixed length array.

I am still dubious I could reuse existing codes. They assume each value could be resolved in-place.

I propose this for the series of D145937. I supposed this was essential to them at first,
but I have noticed list indexing could be hacked with !listconcat !foldl, etc.
I could detach this out of D145937, if this would be supposed not ready for landing.


I have taken BinOpInit for them rather than a new TypedInit derivative, since they can be treated as operators with two values except that they are not bang operators.
I suppose it would make sense if OpInit tree might be handled as the expr tree (like other languages).

I could rewrite them as derivatives of TypeInit if OpInit should be bang operator as principle. Then I can guess they would be code duplication. I think it would not be smart.

@Paul-C-Anagnostopoulos @nhaehnle Thoughts? Could we expand OpInit for non-bang-ops?


I suppose it to interpret 0-7. Tokenized as 0 and -7.
See also;

1 - -2 is tokenized as num:1 minus num:-2.
--2 as minus num:-2.

chapuni updated this revision to Diff 510306.Apr 1 2023, 11:59 PM
chapuni edited the summary of this revision. (Show Details)
  • Detach D147401 as a pre-commit testcase
chapuni edited reviewers, added: nhaehnle; removed: barannikov88.Apr 2 2023, 12:00 AM
chapuni updated this revision to Diff 510311.Apr 2 2023, 12:33 AM
  • Update error messages

I haven't looked at the implementation, but the overall idea seems quite reasonable.

347–348 ↗(On Diff #510311)

Should the "a-b" syntax be supported here at all? It may be time to get serious about deprecating it. Excising all the uses of "a-b" for bit slices would be tough, but I'd think that with lists we're in a better position?

FYI, list[m-n] is used in 77 lines;
list[m - n] is not used!

clang/include/clang/Basic/ |   4
llvm/lib/Target/ARM/     | 118
llvm/lib/Target/ARM/      |  32
3 files changed, 77 insertions(+), 77 deletions(-)

We could revise them, though, I think we don't need to do.

Bitslices are used in many (hundreds? thousands?) places, as you know, and fixing them would be ugly due to replacing one character - to three characters ....

347–348 ↗(On Diff #510311)

We could deprecate them here, though, I think we may leave old syntax,
if TGParser::ParseSliceElement() would be applied also to bitslice.

Deprecating would not gain the big win, I guess.

chapuni updated this revision to Diff 511968.Apr 9 2023, 2:25 AM
  • Use early break
chapuni updated this revision to Diff 516804.Apr 25 2023, 7:55 AM

Is this ready for commit soon?
Or I would commit D146915 with not this but hacky ListAt<list,idx> and detach this from the series.

Let me know whatever I have to do. Thank you.

arsenm accepted this revision.Apr 25 2023, 7:01 PM
arsenm added inline comments.
527 ↗(On Diff #507386)

I'm not sure, I have a hard time judging what the rendered output of this will look like


demorgan the condition

This revision is now accepted and ready to land.Apr 25 2023, 7:01 PM
chapuni updated this revision to Diff 517153.Apr 26 2023, 6:45 AM
  • Rewrite
chapuni marked an inline comment as done.Apr 26 2023, 6:48 AM

Style fixes in LISTELEM and LISTSLICE.

This revision was landed with ongoing or failed builds.Apr 26 2023, 7:50 AM
This revision was automatically updated to reflect the committed changes.