This is an archive of the discontinued LLVM Phabricator instance.

TableGen: Allow 'a+b' in TableGen language
AbandonedPublic

Authored by javed.absar on Oct 4 2019, 3:15 AM.

Details

Summary

!add(a,b) is often used in td files but its bit cumbersome.
This patch allows one to write 'a+b'. !add(a,b) will still work.

Diff Detail

Event Timeline

javed.absar created this revision.Oct 4 2019, 3:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2019, 3:15 AM
hfinkel added a subscriber: hfinkel.Oct 4 2019, 8:54 AM

How far down this road can we go? I would like to see infix operators in TableGen because I believe that it will make it easier to use. However, I'm concerned about just adding add as a special case. Of the other similar operators:

case ADD: Result = "!add"; break;
case MUL: Result = "!mul"; break;
case AND: Result = "!and"; break;
case OR: Result = "!or"; break;
case SHL: Result = "!shl"; break;
case SRA: Result = "!sra"; break;
case SRL: Result = "!srl"; break;
case EQ: Result = "!eq"; break;
case NE: Result = "!ne"; break;
case LE: Result = "!le"; break;
case LT: Result = "!lt"; break;
case GE: Result = "!ge"; break;
case GT: Result = "!gt"; break;

how many can we add as infix operators without ambiguities or other difficulties?

llvm/lib/TableGen/TGParser.cpp
2104

This syntax comment here needs to be updated.

How far down this road can we go? I would like to see infix operators in TableGen because I believe that it will make it easier to use. However, I'm concerned about just adding add as a special case. Of the other similar operators:

case ADD: Result = "!add"; break;
case MUL: Result = "!mul"; break;
case AND: Result = "!and"; break;
case OR: Result = "!or"; break;
case SHL: Result = "!shl"; break;
case SRA: Result = "!sra"; break;
case SRL: Result = "!srl"; break;
case EQ: Result = "!eq"; break;
case NE: Result = "!ne"; break;
case LE: Result = "!le"; break;
case LT: Result = "!lt"; break;
case GE: Result = "!ge"; break;
case GT: Result = "!gt"; break;

how many can we add as infix operators without ambiguities or other difficulties?

I have a half baked shunting-algorithm based infix-operator patch to work with cases you mention but that is kind of major work. I thought maybe, as !add is most commonly used, there maybe an appetite for a simple solution like here.

How far down this road can we go? I would like to see infix operators in TableGen because I believe that it will make it easier to use. However, I'm concerned about just adding add as a special case. Of the other similar operators:

case ADD: Result = "!add"; break;
case MUL: Result = "!mul"; break;
case AND: Result = "!and"; break;
case OR: Result = "!or"; break;
case SHL: Result = "!shl"; break;
case SRA: Result = "!sra"; break;
case SRL: Result = "!srl"; break;
case EQ: Result = "!eq"; break;
case NE: Result = "!ne"; break;
case LE: Result = "!le"; break;
case LT: Result = "!lt"; break;
case GE: Result = "!ge"; break;
case GT: Result = "!gt"; break;

how many can we add as infix operators without ambiguities or other difficulties?

I have a half baked shunting-algorithm based infix-operator patch to work with cases you mention but that is kind of major work. I thought maybe, as !add is most commonly used, there maybe an appetite for a simple solution like here.

To be clear, I'm fine with taking an incremental approach here. It sounds like you've investigated this and there's no major technical impediment, it's just more complicated (and, thus, a more-complicated patch) to deal with all of the operators. Is that correct?

How far down this road can we go? I would like to see infix operators in TableGen because I believe that it will make it easier to use. However, I'm concerned about just adding add as a special case. Of the other similar operators:

case ADD: Result = "!add"; break;
case MUL: Result = "!mul"; break;
case AND: Result = "!and"; break;
case OR: Result = "!or"; break;
case SHL: Result = "!shl"; break;
case SRA: Result = "!sra"; break;
case SRL: Result = "!srl"; break;
case EQ: Result = "!eq"; break;
case NE: Result = "!ne"; break;
case LE: Result = "!le"; break;
case LT: Result = "!lt"; break;
case GE: Result = "!ge"; break;
case GT: Result = "!gt"; break;

how many can we add as infix operators without ambiguities or other difficulties?

I have a half baked shunting-algorithm based infix-operator patch to work with cases you mention but that is kind of major work. I thought maybe, as !add is most commonly used, there maybe an appetite for a simple solution like here.

To be clear, I'm fine with taking an incremental approach here. It sounds like you've investigated this and there's no major technical impediment, it's just more complicated (and, thus, a more-complicated patch) to deal with all of the operators. Is that correct?

absolutely right, Hal.

This doesn't raise any concerns with me but i don't believe i'm most qualified to review this :)
Missing llvm/docs/TableGen/LangIntro.rst, llvm/docs/TableGen/LangRef.rst, llvm/utils/kate/llvm-tablegen.xml changes.

llvm/lib/TableGen/TGParser.cpp
2185–2187

Do you want to make any sanity checks about types of lhs, rhs, final type?

nhaehnle added a subscriber: nhaehnle.EditedOct 8 2019, 6:50 AM

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.

llvm/lib/TableGen/TGParser.cpp
2185–2187

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.

lebedev.ri requested changes to this revision.Oct 16 2019, 10:55 AM

(outstanding review notes not addressed)

This revision now requires changes to proceed.Oct 16 2019, 10:55 AM
javed.absar marked an inline comment as done.

updated based on review comments

Thanks. Any thoughts on the higher-level algorithmic questions?

javed.absar abandoned this revision.Oct 29 2019, 3:52 AM

Abandoning as I think I will try a more holistic approach.