This is an archive of the discontinued LLVM Phabricator instance.

TableGen: Let expressions available to list subscripts and list slices
ClosedPublic

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

Details

Summary

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

chapuni created this revision.Mar 12 2023, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 7:46 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
chapuni requested review of this revision.Mar 12 2023, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 7:46 AM
chapuni retitled this revision from TableGen: Let expressions available to list subscriptions and list slices to TableGen: Let expressions available to list subscripts and list slices.Mar 14 2023, 7:27 AM
chapuni added a reviewer: Paul-C-Anagnostopoulos.
chapuni updated this revision to Diff 505086.Mar 14 2023, 7:29 AM
  • - Add a testcase str[0]
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.
llvm/docs/TableGen/ProgRef.rst
526

This section is indented further than the others

529

Typo 'tyep'

llvm/lib/TableGen/TGParser.cpp
760

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.
llvm/docs/TableGen/ProgRef.rst
526

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.

llvm/lib/TableGen/TGParser.cpp
760

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.
llvm/docs/TableGen/ProgRef.rst
348

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

llvm/include/llvm/TableGen/Record.h
852

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

llvm/lib/TableGen/TGParser.cpp
746

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

788–789

Please give entities descriptive / canonical names.

2655–2658

The whole comment should be synced with the documentation.

2698

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

2701–2702

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

Thanks for reviews! I will fix them later.

llvm/docs/TableGen/ProgRef.rst
348

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

llvm/include/llvm/TableGen/Record.h
852

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

llvm/lib/TableGen/TGParser.cpp
746

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

788–789

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

2655–2658

Excuse me, I don't understand the point.

2698

Understood. I will also try finding error cases.

2701–2702

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?

llvm/docs/TableGen/ProgRef.rst
348

You can update the comment below.

llvm/include/llvm/TableGen/Record.h
852

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.

llvm/lib/TableGen/TGParser.cpp
746

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

788–789

How about Elems and Lists?

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

2655–2658

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.

2701–2702

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/ListSlices-fail.td: 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.

llvm/include/llvm/TableGen/Record.h
852

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?

llvm/lib/TableGen/TGParser.cpp
746

I suppose it to interpret 0-7. Tokenized as 0 and -7.
See also;
https://llvm.org/docs/TableGen/ProgRef.html#id10
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/ARM/ARMScheduleA9.td#L2109

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.

llvm/docs/TableGen/ProgRef.rst
347–348

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/riscv_vector.td |   4
llvm/lib/Target/ARM/ARMScheduleA57.td     | 118
llvm/lib/Target/ARM/ARMScheduleA9.td      |  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 ....

llvm/docs/TableGen/ProgRef.rst
347–348

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.
llvm/docs/TableGen/ProgRef.rst
526

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

llvm/lib/TableGen/Record.cpp
1210

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.