This is an archive of the discontinued LLVM Phabricator instance.

TableGen: Introduce `!range` operator for half-opened interval
ClosedPublic

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

Details

Summary

!range(a, b) generates a list [a,b). a is optional and 0 by default.

  • !range(-1, 4) generates [-1, 0, 1, 2, 3]
  • !range(4) generates [0, 1, 2, 3]
  • !range(2, 2) generates []<list<int>>

!range(list) is equivalent to !range(0, !size(list)).

Diff Detail

Event Timeline

chapuni created this revision.Mar 12 2023, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 7:44 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
chapuni requested review of this revision.Mar 12 2023, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 7:44 AM
chapuni updated this revision to Diff 505088.Mar 14 2023, 7:30 AM
  • Add a test case

I'm tempted to suggest that this bang operator should be called !rangelist. The name !range suggests a pair of integers or a sequence of integers not encapsulated in a list. Add a couple of other reviewers so we can get a second opinion.

Add some simpler initial tests to range-op.td, such as the examples you gave in the summary.

Thanks for your comment.

!range(m, n) and !range(n) come from Python.
!range(list) is just a syntax sugar.

I will accept even if a minimal functionality (m, n) would be landed. Do you think I should drop other syntax sugars?

Add a couple of other reviewers so we can get a second opinion.

I will happy if you could nominate anyone, since you ask a second opinion.

Add some simpler initial tests to range-op.td, such as the examples you gave in the summary.

I didn't think I could add simpler examples to the test. I will do if it would be better.

I've added two reviewers. Could I get a second opinion on the name of this bang operator?

I think a few simpler tests first is a good idea. In particular, it helps a future developer understand what is going on.

chapuni updated this revision to Diff 505477.Mar 15 2023, 7:05 AM
  • Update a test more descriptive

Nice new tests. Thanks!

!range makes sense to me because I also see the analogy to Python's range.

I have some minor notes inline, apart from that the code looks good to me.

llvm/lib/TableGen/TGParser.cpp
1509

You're relying here on the cast always being successful (which makes sense), so you should use cast instead of dyn_cast. (This has the benefit of raising an assertion if the cast fails.)

1510

Using a switch for this instead of an if/else chain is a bit unorthodox, but I suppose we can live with it.

1526

Also here: use cast instead of dyn_cast

I see the analogy to Python's range(), except that it returns a sequence of numbers, not a list. But I'm not really too worried about this.

chapuni updated this revision to Diff 505800.Mar 16 2023, 7:12 AM
  • Use cast<ty> rather than dyn_cast<ty> when <ty> is assumed.
  • Add a testcase.
  • Other cleanups
chapuni marked 3 inline comments as done.Mar 16 2023, 7:22 AM
chapuni added inline comments.
llvm/lib/TableGen/TGParser.cpp
1509

I have misunderstood cast<ty> as it would crash with incompatible type.
Thanks for pointing out.

1510

I avoided duplicate usages of InitList.size(), even if they were optimized out.

arsenm added a subscriber: arsenm.Mar 30 2023, 11:13 AM
arsenm added inline comments.
llvm/lib/TableGen/TGParser.cpp
1527

I still see separate isa and cast?

llvm/test/TableGen/range-op-fail.td
27

Test with mixed integer and not-integer

chapuni updated this revision to Diff 509823.Mar 30 2023, 2:45 PM
chapuni marked 2 inline comments as done.
  • Make Arg1 consistent

@arsenm Thanks for your reviews!

llvm/lib/TableGen/TGParser.cpp
1527

The inner cast was for TypedInit::getType(). Anyways, I have rewritten more consistent.

llvm/test/TableGen/range-op-fail.td
27

This detects that 1st arg is not integer.
OTOH, I suppose ERR1 would cover if 1st arg was integer.

arsenm added inline comments.Apr 7 2023, 3:52 PM
llvm/lib/TableGen/Record.cpp
1206

Can early break and reduce indentation

llvm/test/TableGen/range-op-fail.td
27

Right but you aren't testing the error is produced for each position individually

chapuni updated this revision to Diff 511967.Apr 9 2023, 2:22 AM
  • Use early break
  • range-op-fail.td: Let unnumbered
  • Add ERR_UNEXPECTED_TYPE(s)
chapuni added inline comments.Apr 9 2023, 2:25 AM
llvm/lib/TableGen/Record.cpp
1206

I followed old code style around. Updated.
(Frankly said, I started from copypaste)

llvm/test/TableGen/range-op-fail.td
27

Put string for each position.
(and rename cases as unnumbered)

Ping. Any other issues?

arsenm accepted this revision.Apr 24 2023, 12:09 PM
This revision is now accepted and ready to land.Apr 24 2023, 12:09 PM

I don't see the documentation for this new operator. Did it go in already?

@Paul-C-Anagnostopoulos I suppose ProgRef.rst would satisfy. Would I miss anything?

Yes, that's the file. You need to add the name to the list in section 1.3.3 and the description to section 1.10.

chapuni updated this revision to Diff 516553.Apr 24 2023, 3:32 PM
  • Rebase
  • ProgRef.rst: Update bang ops index

I can't figure out what is going on here. It doesn't look like you added a description of !range.

chapuni added inline comments.Apr 24 2023, 4:01 PM
llvm/docs/TableGen/ProgRef.rst
1780–1787

@Paul-C-Anagnostopoulos Is it insufficient or improper?

llvm/docs/TableGen/ProgRef.rst
1780–1787

Say: The result is ...
If *a* `>=` *b*, then the result is ...

chapuni added inline comments.Apr 24 2023, 4:29 PM
llvm/docs/TableGen/ProgRef.rst
1780–1787

How about them?

  • Omit the sentence "Its result is...", since it is obvious.
  • Revise the last sentence as your suggestion.
llvm/docs/TableGen/ProgRef.rst
1780–1787

I would keep both sentences about the results.

chapuni updated this revision to Diff 516574.Apr 24 2023, 4:38 PM
  • ProgRef.rst: Revise the description
This revision was landed with ongoing or failed builds.Apr 25 2023, 6:38 AM
This revision was automatically updated to reflect the committed changes.
foad added a subscriber: foad.May 2 2023, 2:06 AM

Late naming suggestion: !iota

But I am also fine with !range