[PATCH] D68453: TableGen: Allow 'a+b' in TableGen language

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

This does seem like a useful addition (heh) to the grammar. There is one thing that can go wrong in the future: parentheses are already reserved for DAGs. Brackets and braces also already have their own purpose. We could designate `((` without space for parenthesis, with the risk that `( (` and `((` can mean different things, with the first one making sense in DAGs. Though DAG operators are usually defs, so the risk of weirdness may be acceptable.

I also agree that a shunting-based algorithm is the correct direction to go in long-term, and I'm concerned about adding code that adds momentum in the wrong direction. Right now, it's still easy to turn back and do things right, but if we add the change as-is the temptation will be great for the next person to come along and add even more code that goes off into the woods. Can we please get this right **now**? We can treat `#` and `+` as left-associative operators of the same precedence right now (and don't add anything else), which should keep things somewhat simpler, but at least we should be able to avoid using recursion for parsing the RHS.



================
Comment at: llvm/lib/TableGen/TGParser.cpp:2184-2186
+      Init *RHSResult = ParseValue(CurRec, ItemType, ParseNameMode);
+      Result = BinOpInit::get(BinOpInit::ADD, LHS, RHSResult,
+                                    IntRecTy::get())->Fold(CurRec);
----------------
lebedev.ri wrote:
> Do you want to make any sanity checks about types of lhs, rhs, final type?
Yes. `resolveTypes` should be used on the LHS and RHS types and the result used as the type. That makes it consistent with `!add` handling.

You don't particularly need to check that it's an IntRecTy as far as I'm concerned.


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

https://reviews.llvm.org/D68453