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

[NAKAMURA Takumi via Phabricator via llvm-commits]

chapuni added a reviewer: barannikov88.
chapuni added a comment.

Thanks for reviews! I will fix them later.



================
Comment at: llvm/docs/TableGen/ProgRef.rst:348
+               :| `Value` "-" `Value`
+               :| `Value` `TokInteger`(negative)
 
----------------
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.


================
Comment at: llvm/include/llvm/TableGen/Record.h:852
+    LISTSLICE,
+    RANGEC,
     STRCONCAT,
----------------
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?


================
Comment at: llvm/lib/TableGen/TGParser.cpp:746
+  }
+  case tgtok::IntVal: { // Deprecated "-num"
+    auto i = -Lex.getCurIntVal();
----------------
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)` ?


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


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


================
Comment at: llvm/lib/TableGen/TGParser.cpp:2718
+      auto *LHS = dyn_cast<TypedInit>(Result);
+      assert(LHS);
+      auto *LHSTy = dyn_cast<ListRecTy>(LHS->getType());
----------------
barannikov88 wrote:
> ParseSimpleValue is not guaranteed to return TypedInit, this should be an error.
> 
Understood. I will also try finding error cases.


================
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;
----------------
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"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145872