Replace TableGen range piece punctuator with '..'

[Paul-C-Anagnostopoulos]

Summary

The TableGen range piece punctuator is currently '-' (e.g., {0-9}), which interacts oddly with the fact that an integer literal's sign is part of the literal. This patch replaces the '-' with the new punctuator '..'. The '-' punctuator is deprecated.

[dblaikie]

I don't really work with tablegen, so someone else will need to review the substance of this change.

One nit (provided inline) and one general question: does the '-' range separator produce any usability problems for users of tablegen, or does it only complicate the implementation? (though, admittedly, {-5--2} isn't terribly readable, so even if it works, I can see usability benefit in changing to {-5..-2})

llvm/lib/TableGen/TGLexer.cpp

188–189

Avoid else after return: https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

[Paul-C-Anagnostopoulos]

In D85585#2208140, @dblaikie wrote:

I don't really work with tablegen, so someone else will need to review the substance of this change.

One nit (provided inline) and one general question: does the '-' range separator produce any usability problems for users of tablegen, or does it only complicate the implementation? (though, admittedly, {-5--2} isn't terribly readable, so even if it works, I can see usability benefit in changing to {-5..-2})

The primary purpose of the change is to remove a rather odd bit of syntax that requires an almost apologetic description in the documentation.

Negative numbers aren't allowed in ranges, so the worst use of the hyphen would be in something like {2-+5}. But, again, it's mainly to clean up the documentation.

llvm/lib/TableGen/TGLexer.cpp

188–189

Ah, okay. I'll remove the else and drop through to the second return.

[dblaikie]

In D85585#2208321, @Paul-C-Anagnostopoulos wrote:

In D85585#2208140, @dblaikie wrote:

I don't really work with tablegen, so someone else will need to review the substance of this change.

One nit (provided inline) and one general question: does the '-' range separator produce any usability problems for users of tablegen, or does it only complicate the implementation? (though, admittedly, {-5--2} isn't terribly readable, so even if it works, I can see usability benefit in changing to {-5..-2})

The primary purpose of the change is to remove a rather odd bit of syntax that requires an almost apologetic description in the documentation.

Seems like the comment would still be needed there, since the syntax rule is still there?

& I'm not sure I understand the text in the spec either - does the (pre-this-patch) first rule never fire, oh, it only fires in examples like the one you've given below "{2-+5}" but in "{2-3}" the second rule fires? Perhaps the first rule should be removed (disallowing "{2-+5}" and instead requiring it be written as "{2-5}") & just leave the second. But it does seem like documenting a weird mostly-implementation detail that still does the right thing so far as the user's concerned? Perhaps the syntax description could be written differently? Guess not if the sign is included in the integer token, then there's no way to separate out just the digit portion and say digit+ "-" digit+.

Negative numbers aren't allowed in ranges, so the worst use of the hyphen would be in something like {2-+5}. But, again, it's mainly to clean up the documentation.

Without removing the old feature, I'm not sure it achieves that?

[Paul-C-Anagnostopoulos]

Even though the old syntax would be deprecated, I think it still needs to be in the documentation. I'm writing a new Programmer's Reference and would be happy to eliminate it, but that doesn't seem right. However, it might make sense to separate the two productions and put them with the deprecation warning, in order to help clean up the documentation. Or even make them a footnote.

The code is rather convoluted, because the lexer lexes '0-5' as '0' and '-5'. This is because leading signs are lexed as part of the integer rather than as unary operators. But it also has to handle '0-+5', which is lexed as '0', '-', and '+5'.

You could invalidate the second form, but then the code would have to check for it and complain, so nothing would be gained.

[dblaikie]

I'm by no means a decider here - I haven't written tablegen files (except the really basic ones in Clang) & certainly don't maintain it in any way - just my 2c that maybe this isn't worth it if it's not going to remove the old one? (& even then, I imagine the documentation for the current behavior could be written in a way that still essentially describes the behavior users rely on (if it was written as "integer - integer" do you think anyone would care that that's not how it's lexed and that they couldn't write "-5--3"? Especially since negative values aren't allowed anyway) & not be particularly confusing/problematic?)

But I'll leave it to folks with more tablegen context to hash this out - just figured I might be able to get the ball rolling a bit in the mean time.

[Paul-C-Anagnostopoulos]

I just changed the new documentation by removing the two "weird" productions and using a warning to describe the old syntax and state that it is deprecated. That seems like a good compromise that I could apply to the old documentation, too.

Perhaps I'm being pushier than is warranted. The hyphen just seems so weird, especially when so many other languages use '..' (and as a copyeditor I would correct a hyphen if it were used in a range).

I suspect we could eventually remove the old form. Is there a deprecation => removal policy for LLVM?

[Paul-C-Anagnostopoulos]

I have incorporated David's suggestions.

[lattner]

This looks good to me, but please change this to "..." instead of "..". Also, in the documentation, please mention that this is an *inclusive* range. Thank you for working on this!

llvm/lib/TableGen/TGLexer.h

46

The token name should follow their visual look, not their usage. This should be named "periodperiod" or perhaps "dotdot" (if so, change the "period" enum to "dot")

[Paul-C-Anagnostopoulos]

In D85585#2219898, @lattner wrote:

This looks good to me, but please change this to "..." instead of "..". Also, in the documentation, please mention that this is an *inclusive* range. Thank you for working on this!

You're quite welcome. I will change it to '...' and call it "dotdotdot". I chose "range" because the token right above it is called "paste" rather than "sharp".

[lattner]

Makes sense - in a follow-on patch, it would be great to fix those up too, thanks!

[Paul-C-Anagnostopoulos]

I changed the new punctuator to '...' and named it 'dotdotdot'. I renamed the 'period' punctuator to 'dot'. I will rename the 'paste' punctuator to 'sharp' in a future patch.

[nhaehnle]

I like this. I agree with David that the goal should be to remove existing uses of the old syntax, but I think it's fine to do that in a separate patch. One small nit, apart from that LGTM.

llvm/lib/TableGen/TGParser.cpp

674

Needs to be updated to '...'

[Paul-C-Anagnostopoulos]

There was one comment where I neglected to change '..' to '...'. Fixed now.

@nhaehnle, could you commit this for me?

[lattner]

nice! thank you

[MaskRay]

llvm/lib/TableGen/TGParser.cpp

727

Given the new syntax ..., would be better deprecating the reversed range behavior?

[Paul-C-Anagnostopoulos]

This might make sense in the future, once all uses of the deprecated '-' syntax is gone. Although I'm not sure why '...' necessarily implies a particular order.

llvm/docs/ReleaseNotes.rst

 * ...
 
 * Added the ``byref`` attribute to better represent argument passing
   for the `amdgpu_kernel` calling convention.
 
 Changes to building LLVM
 ------------------------
 
+Changes to TableGen
+-------------------
+
+* The syntax for specifying an integer range in a range list has changed.
+  The old syntax used a hyphen in the range (e.g., ``{0-9}``). The new syntax
+  uses the "`...`" range punctuator (e.g., ``{0...9}``). The hyphen syntax
+  is deprecated. The "TableGen Language Reference" document has been updated.
+
 Changes to the ARM Backend
 --------------------------
 
 During this release ...
 
 Changes to the MIPS Target
 --------------------------
 

llvm/docs/TableGen/ProgRef.rst

 
 .. productionlist::
    Value: `SimpleValue` `ValueSuffix`*
    ValueSuffix: "{" `RangeList` "}"
               :| "[" `RangeList` "]"
               :| "." `TokIdentifier`
    RangeList: `RangePiece` ("," `RangePiece`)*
    RangePiece: `TokInteger`
-             :| `TokInteger` ".." `TokInteger`
+             :| `TokInteger` "..." `TokInteger`
              :| `TokInteger` "-" `TokInteger`
              :| `TokInteger` `TokInteger`
 
 .. warning::
   The peculiar last form of :token:`RangePiece` is due to the fact that the
   "``-``" is included in the :token:`TokInteger`, hence ``1-5`` gets lexed as
   two consecutive tokens, with values ``1`` and ``-5``,
   instead of "1", "-", and "5".

llvm/lib/TableGen/TGLexer.h

enum TokKind {
 
     // Tokens with no info.
     minus, plus,        // - +
     l_square, r_square, // [ ]
     l_brace, r_brace,   // { }
     l_paren, r_paren,   // ( )
     less, greater,      // < >
     colon, semi,        // : ;
-    comma, period,      // , .
+    comma, dot,         // , .
     equal, question,    // = ?
     paste,              // #
+    dotdotdot,          // ...
 
     // Keywords. ('ElseKW' is named to distinguish it from the existing 'Else'
     // that means the preprocessor #else.)
     Bit, Bits, Class, Code, Dag, Def, Foreach, Defm, Field, In, Int, Let, List,
     MultiClass, String, Defset, Defvar, If, Then, ElseKW,
 
     // !keywords.
     XConcat, XADD, XMUL, XAND, XOR, XSRA, XSRL, XSHL, XListConcat, XListSplat,

llvm/lib/TableGen/TGLexer.cpp

case EOF:
     if (processEOF())
       return LexToken();
 
     // Return EOF denoting the end of lexing.
     return tgtok::Eof;
 
   case ':': return tgtok::colon;
   case ';': return tgtok::semi;
-  case '.': return tgtok::period;
   case ',': return tgtok::comma;
   case '<': return tgtok::less;
   case '>': return tgtok::greater;
   case ']': return tgtok::r_square;
   case '{': return tgtok::l_brace;
   case '}': return tgtok::r_brace;
   case '(': return tgtok::l_paren;
   case ')': return tgtok::r_paren;
   case '=': return tgtok::equal;
   case '?': return tgtok::question;
   case '#':
     if (FileOrLineStart) {
       tgtok::TokKind Kind = prepIsDirective();
       if (Kind != tgtok::Error)
         return lexPreprocessor(Kind);
     }
 
     return tgtok::paste;
 
+  // The period is a separate case so we can recognize the "..."
+  // range punctuator.
+  case '.':
+    if (peekNextChar(0) == '.') {
+      ++CurPtr; // Eat second dot.
+      if (peekNextChar(0) == '.') {
+        ++CurPtr; // Eat third dot.
+        return tgtok::dotdotdot;
+      }
+      return ReturnError(TokStart, "Invalid '..' punctuation");
+    }
+    return tgtok::dot;
+
   case '\r':
     PrintFatalError("getNextChar() must never return '\r'");
     return tgtok::Error;
 
   case 0:
   case ' ':
   case '\t':
     // Ignore whitespace.

llvm/lib/TableGen/TGParser.cpp

ParseSubMultiClassReference(MultiClass *CurMC) {
   }
   Result.RefRange.End = Lex.getLoc();
 
   return Result;
 }
 
 /// ParseRangePiece - Parse a bit/value range.
 ///   RangePiece ::= INTVAL
+///   RangePiece ::= INTVAL '...' INTVAL
 ///   RangePiece ::= INTVAL '-' INTVAL
-///   RangePiece ::= INTVAL INTVAL
+///   RangePiece ::= INTVAL INTVAL 
+// The last two forms are deprecated.
 bool TGParser::ParseRangePiece(SmallVectorImpl<unsigned> &Ranges,
                                TypedInit *FirstItem) {
   Init *CurVal = FirstItem;
   if (!CurVal)
     CurVal = ParseValue(nullptr);
 
   IntInit *II = dyn_cast_or_null<IntInit>(CurVal);
   if (!II)
     return TokError("expected integer or bitrange");
 
   int64_t Start = II->getValue();
   int64_t End;
 
   if (Start < 0)
     return TokError("invalid range, cannot be negative");
 
   switch (Lex.getCode()) {
   default:
     Ranges.push_back(Start);
     return false;
+
+  case tgtok::dotdotdot:
   case tgtok::minus: {
     Lex.Lex(); // eat
 
     Init *I_End = ParseValue(nullptr);
     IntInit *II_End = dyn_cast_or_null<IntInit>(I_End);
     if (!II_End) {
       TokError("expected integer value as end of range");
       return true;
bool TGParser::ParseRangePiece(SmallVectorImpl<unsigned> &Ranges,
   if (End < 0)
     return TokError("invalid range, cannot be negative");
 
   // Add to the range.
   if (Start < End)
     for (; Start <= End; ++Start)
       Ranges.push_back(Start);
   else
     for (; Start >= End; --Start)
       Ranges.push_back(Start);
   return false;
 }
 
 /// ParseRangeList - Parse a list of scalars and ranges into scalar values.
 ///
 ///   RangeList ::= RangePiece (',' RangePiece)*
 ///
case tgtok::l_square: {
 
       // Eat the ']'.
       if (!consume(tgtok::r_square)) {
         TokError("expected ']' at end of list slice");
         return nullptr;
       }
       break;
     }
-    case tgtok::period: {
+    case tgtok::dot: {
       if (Lex.Lex() != tgtok::Id) {  // eat the .
         TokError("expected field identifier after '.'");
         return nullptr;
       }
       StringInit *FieldName = StringInit::get(Lex.getCurStrVal());
       if (!Result->getFieldType(FieldName)) {
         TokError("Cannot access field '" + Lex.getCurStrVal() + "' of value '" +
                  Result->getAsString() + "'");

llvm/test/TableGen/range-lists.td

+// RUN: llvm-tblgen %s | FileCheck %s
+// XFAIL: vg_leak
+
+// This file has tests for range lists and range pieces.
+
+// These are tests for bits ranges.
+
+def bit_range_hyphen {
+  bits<16> field1;
+  let field1{15, 14, 13, 12} = {1, 0, 1, 0};
+  let field1{11-8} = {1, 0, 1, 1};
+  let field1{+7-4} = {1, 1, 0, 0};
+  let field1{+3-+0} = {1, 1, 0, 1};
+  bit hyphen_field1_ok = !eq(field1, 0xABCD);
+}
+
+def bit_range_dotdotdot {
+  bits<16> field1;
+  let field1{15, 14, 13, 12} = {1, 0, 1, 0};
+  let field1{11...8} = {1, 0, 1, 1};
+  let field1{+7...4} = {1, 1, 0, 0};
+  let field1{+3...+0} = {1, 1, 0, 1};
+  bit dotdotdot_field1_ok = !eq(field1, 0xABCD);
+}
+
+if !eq(bit_range_hyphen.field1, bit_range_dotdotdot.field1) then
+  def bit_range_ok {}
+else
+  def bit_range_not_ok {}
+
+// These are tests for lists.
+
+def list_range_hyphen {
+  list<string> field1 = ["foo", "bar", "baz", "snork", "quux", "quuux",
+                         "bazola", "ztesch", "bletch", "flarp"];
+  list<string> subfielda = field1[0, 1, 2, 3];
+  list<string> subfieldb = field1[4-5];
+  list<string> subfieldc = field1[+6-7];
+  list<string> subfieldd = field1[+8-+9];
+  bit hyphen_subfields_ok = !and(!eq(subfieldb[0], "quux"),
+                                 !eq(subfieldd[1], "flarp"));
+}
+
+def list_range_dotdotdot {
+  list<string> field1 = ["foo", "bar", "baz", "snork", "quux", "quuux",
+                         "bazola", "ztesch", "bletch", "flarp"];
+  list<string> subfielda = field1[0, 1, 2, 3];
+  list<string> subfieldb = field1[4...5];
+  list<string> subfieldc = field1[+6...7];
+  list<string> subfieldd = field1[+8...+9];
+  bit dotdotdot_subfields_ok = !and(!eq(subfieldb[0], "quux"),
+                                    !eq(subfieldd[1], "flarp"));
+}
+
+if !eq(!head(list_range_hyphen.subfieldd),
+       !head(list_range_dotdotdot.subfieldd)) then
+  def list_range_ok {}
+else
+  def list_range_not_ok {}
+
+// This is a test of foreach.
+
+foreach i = {0-3} in 
+  foreach j = {4...5} in
+    def eachrec#i#j {
+      int fi = i;
+      int fj = j;
+    }
+
+//CHECK: bit dotdotdot_field1_ok = 1
+//CHECK: bit hyphen_field1_ok = 1
+//CHECK: def bit_range_ok {
+
+//CHECK: def eachrec04 {
+//CHECK: def eachrec35 {
+
+//CHECK: bit dotdotdot_subfields_ok = 1
+//CHECK: bit hyphen_subfields_ok = 1
+//CHECK: def list_range_ok {
+