Page MenuHomePhabricator

[TableGen] : Extend !if semantics through new language feature !ifs
ClosedPublic

Authored by javed.absar on Dec 17 2018, 1:53 AM.

Details

Summary

Currently, some TableGen expressions are forced to use multiple embedded '!if' which makes the the expression cumbersome. Below is an example (from an actual td file):

list<SubRegIndex> ret = !if(!eq(size, 2), ret2,
                                              !if(!eq(size, 3), ret3,
                                                  !if(!eq(size, 4), ret4,
                                                       !if(!eq(size, 8), ret8, ret16))));

This patch will allow such expession to be more easily written as:

list<SubRegIndex> ret =

!ifs(!eq(size, 2) : ret2,
      !eq(size, 3) : ret3,
      !eq(size, 4) : ret4,
      !eq(size, 8) : ret8,
      default: ret16);

The default can appear in the begining, end or anywhere within the !ifs.

The grammer of !ifs is :

SimpleValue ::= IFS '(' [Value ':' Value,]+ DEFAULT ':' Value ')'

We could call 'ifs' 'switch' although it is slightly different from C language switch as the case value is not a constant.
Some of the tests below are ported from !if as those check are required of !ifs as well.

`!ifs(cond1: val1, cond2 : val2, ..., default : valn)`

Instead of embedding !if inside !if which can get cumbersome, one can use !ifs.

!ifs returns 'val1' if the result of 'int' or 'bit' operator 'cond1' is nonzero.

Otherwise, checks 'cond2'. If 'cond2' is nonzero, returns 'val2', and so on.

If all conditions are zero, the default value 'valn' is returned.

The default value can appear anywhere, but there must be

one and only one default value. Below is an example to convert an integer 'x'

into string form:

 

string S =  !ifs(!lt(x,0) : "Negative",

                 !eq(x,0) : "zero",

                 !eq(x,1) : "one",

                 default  : "MoreThanOne");

Diff Detail

Repository
rL LLVM

Event Timeline

javed.absar created this revision.Dec 17 2018, 1:53 AM
javed.absar edited the summary of this revision. (Show Details)
kristof.beyls added inline comments.
docs/TableGen/LangIntro.rst
261–266 ↗(On Diff #178433)

Right now, it seems "!ifs" is somewhat a mix of some syntactic sugar over nested ifs and a switch statement.
The syntax of the example below makes me instinctively read this as something akin a switch statement.
However, the semantics described above indicates clearly that order matters: the first nonzero condition matters.
If you want the logic of nested ifs, maybe it's easier to read to the non-expert Tablegen reader (which I guess is the majority of people reading Tablegen files) if the syntax still reflected nested ifs?

I wonder if it wouldn't be better overall to move to calling this "switch" rather than "ifs", but then also make it an error for more than 1 condition to be non-zero.
That way, the order wouldn't matter.
Would that actually be useful?

javed.absar marked an inline comment as done.Dec 17 2018, 5:37 AM
javed.absar added inline comments.
docs/TableGen/LangIntro.rst
261–266 ↗(On Diff #178433)

I am fine with calling it 'switch' if that is more intuitive to the users. Making it an error for more than one condition to be true - I am not sure if users will like that restriction.

Other option could be to call it !cond( (test1, action1), (test2, action2)...) much like LISP cond - https://www.tutorialspoint.com/lisp/lisp_cond_construct.htm . It is an error if none of the conditions are true, and the 'default' can achieved by ending always with a true clause i.e. (1, actionN)

Eugene.Zelenko set the repository for this revision to rL LLVM.
hfinkel added inline comments.
docs/TableGen/LangIntro.rst
261–266 ↗(On Diff #178433)

FWIW, I like '!cond' better than '!ifs', although I like your original syntax matter than the more LISP-like syntax. Making it an error if none of the conditions are true, and there is no default, makes sense to me. I'd restrict default to appear last, unless there's some reason to do otherwise.

javed.absar marked 2 inline comments as done.EditedDec 19 2018, 2:27 AM
This comment has been deleted.
docs/TableGen/LangIntro.rst
261–266 ↗(On Diff #178433)

Thanks Hal for the suggestion.
I will rework this patch to take following approach:

'!cond' '(' [ Value : Val ]+, DEFAULT: Value ')'

The default must be present and should be last.

Changes based on review comments. The new operator is now called '!cond', and 'default' must be the last clause.

Ping!

Hi Hal/Others:

I have changed the syntax from !ifs to '!cond' now, while retaining the others parts from original !ifs (instead of from LISP). Please let me know if changes are ok with you.

Thanks

hfinkel added inline comments.Jan 4 2019, 1:22 PM
include/llvm/Target/TargetInstrPredicate.td
220 ↗(On Diff #179471)

This is necessary because "default" is now a keyword? Should we name it something else less likely to be used already... what about if we named this 'otherwise' instead of 'default'? Maybe just 'else' would be better? Limiting downstream churn and confusion seems worthwhile. If we have this many things to change upstream, I imagine there are even more downstream .td. files that would break.

simon_tatham added inline comments.Jan 7 2019, 1:27 AM
include/llvm/Target/TargetInstrPredicate.td
220 ↗(On Diff #179471)

I was wondering if a nicer approach to the 'default' problem would be to default to the undefined value if no clause matches, since Tablegen does have that concept already.

That might be all you need in some cases (e.g. if you're confident that one of your explicit conditions should always match anyway). And if you wanted a default of your own you could just make the last argument pair read 1: my_def_value, in exactly the same way that users of Lisp cond often write the last clause as (t my-def-value). Then there's no need to define a new keyword.

If a particular user thinks it's clearer to write default, then perhaps that user could define default in a way that makes it evaluate to true! :-)

greened added inline comments.Jan 7 2019, 10:03 AM
include/llvm/Target/TargetInstrPredicate.td
220 ↗(On Diff #179471)

I like Smon's suggestion here. I have a slight preference for a hard error if no condition matches rather than returning undefined. It think it's pretty common for developers to expect one of the conditions to match and if they don't they'll want a fast failure.

lib/TableGen/Record.cpp
1698 ↗(On Diff #179471)

Rather than CaseRange, consider CondRange. CaseRange could be confusing to developers who look at this in the future, thinking it's a switch statement rather than a generalized if/condition.

simon_tatham added inline comments.Jan 8 2019, 4:28 AM
include/llvm/Target/TargetInstrPredicate.td
220 ↗(On Diff #179471)

I hadn't actually considered the possibility of a hard error, but now you suggest it, I agree that it would be better, not least because it strictly increases the functionality. No other suggested design even offers the option of a hard error, and you're clearly right that it's something that at least some users are likely to want. So with your plan, a user can get a hard error if they want one, or put a default of their choice in front of it if they don't.

Hi all:
I have made the changes as requested. That is, removed explicit 'default' while sticking to overall syntax as before. I found that removing default in fact simplifies the implementation somewhat. Also, have split the tests to cleaner separate ones.

javed.absar marked 2 inline comments as done.Jan 16 2019, 5:03 AM
simon_tatham added inline comments.Jan 17 2019, 9:30 AM
docs/TableGen/LangIntro.rst
261 ↗(On Diff #182002)

I've just remembered that docs/TableGen/LangRef.rst contains a BNF description of the entire TableGen grammar, which should be updated if you're adding a new !foo operator at all (it includes a list of them all in the production for BangOperator). In this case, your new operator doesn't even have the same syntax as the rest of them, because of the unprecedented colon that separates the halves of each case, so it'll need a whole extra grammar rule :-)

Added BNF grammer for !cond as suggested .

javed.absar marked an inline comment as done.Jan 22 2019, 9:16 AM
This revision is now accepted and ready to land.Jan 24 2019, 10:45 AM
This revision was automatically updated to reflect the committed changes.