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

[David Greene via Phabricator via llvm-commits]

greened added inline comments.


================
Comment at: include/llvm/Target/TargetInstrPredicate.td:220
 class MCOpcodeSwitchStatement<list<MCOpcodeSwitchCase> cases,
-                              MCStatement default> : MCStatement {
+                              MCStatement defaultcase> : MCStatement {
   list<MCOpcodeSwitchCase> Cases = cases;
----------------
simon_tatham wrote:
> hfinkel wrote:
> > 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.
> > 
> > 
> 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! :-)
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.


================
Comment at: lib/TableGen/Record.cpp:1698
+static void ProfileCondOpInit(FoldingSetNodeID &ID,
+                             ArrayRef<Init *> CaseRange,
+                             ArrayRef<Init *> ValRange,
----------------
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.


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

https://reviews.llvm.org/D55758