[PATCH] D145872: TableGen: Let expressions available to list subscripts and list slices

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 resigned from this revision.
barannikov88 added a subscriber: nhaehnle.
barannikov88 added a comment.

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?



================
Comment at: llvm/docs/TableGen/ProgRef.rst:348
+               :| `Value` "-" `Value`
+               :| `Value` `TokInteger`(negative)
 
----------------
chapuni wrote:
> barannikov88 wrote:
> > IIUC this is supposed to be normative and `(negative)` is not normative.
> > 
> I could drop `(negative)` if you mention the notation.
> I know this is less strict but I wanted to explain semantic here.
You can update the comment below.


================
Comment at: llvm/include/llvm/TableGen/Record.h:852
+    LISTSLICE,
+    RANGEC,
     STRCONCAT,
----------------
chapuni wrote:
> barannikov88 wrote:
> > What is C in RANGEC? Please give a descriptive name.
> It is a retronym "RANGE (close interval)" to D145871 (`RANGE` as half-open interval).
> Could I revise both names?
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.



================
Comment at: llvm/lib/TableGen/TGParser.cpp:746
+  }
+  case tgtok::IntVal: { // Deprecated "-num"
+    auto i = -Lex.getCurIntVal();
----------------
chapuni wrote:
> barannikov88 wrote:
> > I don't get it how IntVal implies `-`. Could you explain?
> See L754 in `TGParser::ParseRangePiece()`.
> Could I update this as `// Expects "-num" (deprecated)` ?
I think it is because `1 - -2` parses the same as `1 2` because `--2 == 2`.



================
Comment at: llvm/lib/TableGen/TGParser.cpp:788-789
+  TypedInit *CurVal;
+  SmallVector<Init *, 2> LoV;      // int
+  SmallVector<TypedInit *, 2> LoL; // list<int>
+
----------------
chapuni wrote:
> barannikov88 wrote:
> > Please give entities descriptive / canonical names.
> > 
> Will fix. How about `Elems` and `Lists`? I don't prefer longer names.
> How about Elems and Lists?
Elems and Lists would be better if it is what they are.



================
Comment at: llvm/lib/TableGen/TGParser.cpp:2675-2678
 ///   Value       ::= SimpleValue ValueSuffix*
 ///   ValueSuffix ::= '{' BitList '}'
-///   ValueSuffix ::= '[' BitList ']'
+///   ValueSuffix ::= '[' SliceElements ']'
 ///   ValueSuffix ::= '.' ID
----------------
chapuni wrote:
> barannikov88 wrote:
> > The whole comment should be synced with the documentation.
> > 
> Excuse me, I don't understand the point.
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.



================
Comment at: llvm/lib/TableGen/TGParser.cpp:2721-2722
+      if (!LHSTy) {
+        Error(SquareLoc, "Type '" + Twine(LHS->getType()->getAsString()) +
+                             "' is invalid for list subscript");
+        return nullptr;
----------------
chapuni wrote:
> barannikov88 wrote:
> > It should point to the element, not the opening bracket.
> > 
> 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"?
> 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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145872/new/

https://reviews.llvm.org/D145872