tblgen bug in handling case , switch_on

Sanjiv_Gupta · September 5, 2009, 6:46pm

Hi Mikhail,

What is mentioned in the reference manual is this:

    // Evaluates to "cmdline1" if the option "-A" is provided on the
    // command line; to "cmdline2" if "-B" is provided;
    // otherwise to "cmdline3".
     (case
        (switch_on "A"), "cmdline1",
        (switch_on "B"), "cmdline2",
        (default), "cmdline3")

What is generated is this:

     if (A) {
       ...
     }
        if (B) {
       ....
     } else {
       ....
     }

IMO, What should be generated is below:

     if (A) {
       ...
     } else if (B) {
       ....
     } else {
       ....
     }

BTW, to give you more details, I am using append_cmd on each switch_on.

- Sanjiv

Sanjiv_Gupta · September 5, 2009, 7:12pm

Sanjiv Gupta wrote:

Hi Mikhail,

What is mentioned in the reference manual is this:

    // Evaluates to "cmdline1" if the option "-A" is provided on the
    // command line; to "cmdline2" if "-B" is provided;
    // otherwise to "cmdline3".
     (case
        (switch_on "A"), "cmdline1",
        (switch_on "B"), "cmdline2",
        (default), "cmdline3")

What is generated is this:

     if (A) {
       ...
     }
        if (B) {
       ....
     } else {
       ....
     }

   IMO, What should be generated is below:

     if (A) {
       ...
     } else if (B) {
       ....
     } else {
       ....
     }

BTW, to give you more details, I am using append_cmd on each switch_on.

- Sanjiv

_______________________________________________

Is the patch below ok?

Index: LLVMCConfigurationEmitter.cpp

Mikhail_Glushenkov1 · September 5, 2009, 8:32pm

Hi,

Is the patch below ok?

Index: LLVMCConfigurationEmitter.cpp

--- LLVMCConfigurationEmitter.cpp (revision 80668)
+++ LLVMCConfigurationEmitter.cpp (working copy)
@@ -1141,6 +1141,7 @@
Callback, EmitElseIf, OptDescs, O);
}
else {
+ EmitElseIf = true;
Callback(arg, (std::string(IndentLevel) + Indent1).c_str(), O);
}
O << IndentLevel << "}\n";

No, this is not OK - EmitCaseConstructHandler is supposed to be able
to generate both 'if (...) ... if (...) ... if (...) ...' and 'if
(...) ... else if (...) ... else ...' forms. That's what the
EmitElseIf argument controls.

BTW, the example in the documentation generates the 'if (...) ... else
if (...) ... else ...' form since the 'case' there is assumed to be
inside the 'cmd_line' property.

Can you provide a small TableGen code example of what you're trying to
do which is not working?

Sanjiv_Gupta · September 6, 2009, 5:48pm

Mikhail Glushenkov wrote:

Hi,

Is the patch below ok?

Index: LLVMCConfigurationEmitter.cpp

--- LLVMCConfigurationEmitter.cpp (revision 80668)
+++ LLVMCConfigurationEmitter.cpp (working copy)
@@ -1141,6 +1141,7 @@
                              Callback, EmitElseIf, OptDescs, O);
   }
   else {
+ EmitElseIf = true;
     Callback(arg, (std::string(IndentLevel) + Indent1).c_str(), O);
   }
   O << IndentLevel << "}\n";

No, this is not OK - EmitCaseConstructHandler is supposed to be able
to generate both 'if (...) ... if (...) ... if (...) ...' and 'if
(...) ... else if (...) ... else ...' forms. That's what the
EmitElseIf argument controls.

BTW, the example in the documentation generates the 'if (...) ... else
if (...) ... else ...' form since the 'case' there is assumed to be
inside the 'cmd_line' property.

Can you provide a small TableGen code example of what you're trying to
do which is not working?

I wanted to be able to specify -O0, -O1, -O2 on the mcc16 command line.
And -O2 should be default(if none of -O0 or -O1 is specified). So I made following changes to PIC16Base.td, but that didn't work.

def optimizer : Tool<[
(in_language "llvm-bitcode"),
(out_language "llvm-bitcode"),
(output_suffix "bc"),
(cmd_line "$CALL(GetBinDir)opt $INFILE -o $OUTFILE"),
(actions (case
          (switch_on "O0"), (append_cmd "-disable-opt"),
          (switch_on "O1"), (append_cmd "-constmerge -globaldce -globalopt -ipsccp -jump-threading -simplifycfg -gvn -scalarrepl"),
          (switch_on "O2"), (append_cmd "-constmerge -globaldce -globalopt -ipsccp -jump-threading -simplifycfg -gvn -instcombine -scalarrepl"),
          (default), (append_cmd "-constmerge -globaldce -globalopt -ipsccp -jump-threading -simplifycfg -gvn -instcombine -scalarrepl")))
]>;

Mikhail_Glushenkov1 · September 6, 2009, 6:13pm

Hi Sanjiv,

[...]

[Sorry, the formatting was a bit off]

The following snippet gives the expected behaviour (not tested, but
you should get the idea):

(case
(switch_on "O0"),
(append_cmd "-disable-opt"),

    (or (and (switch_on "O1")
             (not (switch_on "O0"))),
        (not (switch_on "O0"))),
    (append_cmd "-constmerge -globaldce -globalopt -ipsccp
-jump-threading -simplifycfg -gvn -scalarrepl"),

    (or (and (switch_on "O2"),
             (not (switch_on "O0"))),
        (not (switch_on "O0"))),
    (append_cmd "-instcombine"))

Mikhail_Glushenkov1 · September 6, 2009, 9:49pm

Hi Sanjiv,

Hi Sanjiv,

[...]

[Sorry, the formatting was a bit off]

The following snippet gives the expected behaviour (not tested, but
you should get the idea):

BTW, your mail has got me thinking about the semantics of 'case',
which is currently somewhat ambiguous (since it depends on context).
Probably 'case' should be modified to always mean 'if ... else if ...
else if ... [...] else ...' and the 'if (...) ... if (...) ... if
(...) ... [...]' form should be called something like 'match'. That
would be backwards-incompatible, though.

What do you think?

Sanjiv_Gupta · September 7, 2009, 6:22am

Mikhail Glushenkov wrote:

Hi Sanjiv,

Hi Sanjiv,

[...]

[Sorry, the formatting was a bit off]

The following snippet gives the expected behaviour (not tested, but
you should get the idea):

BTW, your mail has got me thinking about the semantics of 'case',
which is currently somewhat ambiguous (since it depends on context).
Probably 'case' should be modified to always mean 'if ... else if ...
else if ... [...] else ...'

IMO, we shouldn't think in terms of what code the user wants to generate; The syntax should rather specify what can you do with it. What code we generate for that syntax is rather immaterial as long as it works.
Having said that, a "tblgen case ..... switch_on" is more like a "C switch....case" for me, and a if ... elseif...elseif...else is a valid way to handle it.
Now if we allow nesting, that should work as well.

and the 'if (...) ... if (...) ... if
(...) ... [...]' form should be called something like 'match'. That
would be backwards-incompatible, though.

What do you think?

Again, if an user wants to check for multiple conditions to be true before taking some action , we can devise a new construct for doing that.
A match (this)...and ...match(this) ...and match(this)... makes sense to me.

- Sanjiv

Sanjiv_Gupta · September 10, 2009, 3:57am

Mikhail Glushenkov wrote:

Hi Sanjiv,

Hi Sanjiv,

[...]

[Sorry, the formatting was a bit off]

The following snippet gives the expected behaviour (not tested, but
you should get the idea):

BTW, your mail has got me thinking about the semantics of 'case',
which is currently somewhat ambiguous (since it depends on context).
Probably 'case' should be modified to always mean 'if ... else if ...
else if ... [...] else ...' and the 'if (...) ... if (...) ... if
(...) ... [...]' form should be called something like 'match'. That
would be backwards-incompatible, though.

What do you think?

Another way would be to include a "break" command, to take you after the default label.
- Sanjiv

Sanjiv_Gupta · September 10, 2009, 5:26am

Sanjiv Gupta wrote:

Mikhail Glushenkov wrote:


Hi Sanjiv,

Hi Sanjiv,

[...]


[Sorry, the formatting was a bit off]

The following snippet gives the expected behaviour (not tested, but
you should get the idea):


BTW, your mail has got me thinking about the semantics of 'case',
which is currently somewhat ambiguous (since it depends on context).
Probably 'case' should be modified to always mean 'if ... else if ...
else if ... [...] else ...' and the 'if (...) ... if (...) ... if
(...) ... [...]' form should be called something like 'match'. That
would be backwards-incompatible, though.

What do you think?

Another way would be to include a "break" command, to take you after the default label.
- Sanjiv

Sow we can write:

  case ( (switch_on "O0") [(append_cmd "-blah0"), (break)],
             (switch_on "O1")[(append_cmd "-blah1"), (break)],
             (switch_on "O2")[(append_cmd "-blah2"), (break)],
             (default) (append_cmd "-blah2"))

        This would require generation of an unique label after each case construct, and a goto uniquelabel; in each if ( ) {...}
The other way is:
  if (first_test) {
    ....
    stop_case = true;
  }

  if (!stop_case && second_test) {
    ...
    stop_case = true;
  }

  if (!stop_case && third_test) {
    ...
  } else {
    // default case
    ...
  }
  stop_case = false;

Also, it would be nice to have some "general predicates" to do some cleaning up of the command line.
For example: if an user specifies all -O0, -O1, -O2 on the command line, one would be able to choose only the first or last, or give an error.

option_validator ("O0", "O1", "O2"), (choose_first)
OR
option_validator ("O0", "O1", "O2"), (choose_last)
OR
option_validator ("O0", "O1", "O2"), (error "Only one of -O0, -O1, or -O2 are allowed).

Mikhail_Glushenkov1 · September 10, 2009, 12:25pm

Hi,

Another way would be to include a "break" command, to take you after the
default label.

Yes, this can be useful. I think we should add both 'break' and a
'match' form, and make 'match' the only one allowed in the cmd_line
property. This will make the semantics of 'case' less ambiguous.

Also, it would be nice to have some "general predicates" to do some cleaning
up of the command line.
For example: if an user specifies all -O0, -O1, -O2 on the command line, one
would be able to choose only the first or last, or give an error.

option_validator ("O0", "O1", "O2"), (choose_first)
OR
option_validator ("O0", "O1", "O2"), (choose_last)
OR
option_validator ("O0", "O1", "O2"), (error "Only one of -O0, -O1, or -O2
are allowed).

Good idea. I propose adding something along these lines:

def Preprocess : OptionPreprocessor <[
(squash ["O1", "O2", "O3"], "O3"),
(squash ["O1", "O2"], "O2"),
(conflict (and (switch_on "E"), (switch_on "S"))),
(warning (and (switch_on "E"), (switch_on "S")), "-E conflicts with -S")
]>;

The preprocessing code will run once in the beginning.

Sanjiv_Gupta · September 11, 2009, 9:46am

Mikhail Glushenkov wrote:

Hi,

Why do we need both 'conflict' and 'warning' ?

'warning' just prints a warning, 'conflict' is a fatal error.

A better example would be something like:

(warning (and (switch_on "O1"), (switch_on "O2")) "-O1 has no effect.")

Looks good.
One more quick query.
How to extract libname from "-l std" from the driver and pass it as "std.lib" to the linker tool?
I know that unpack_values will give me "std", but an (append_cmd ".lib") with that will insert a space.
Anything like append_cmd_nospace ? or any other way?

- Sanjiv

Mikhail_Glushenkov1 · September 11, 2009, 12:10pm

Hi,

Looks good.
One more quick query.
How to extract libname from "-l std" from the driver and pass it as
"std.lib" to the linker tool?
I know that unpack_values will give me "std", but an (append_cmd ".lib")
with that will insert a space.

This won't work since actions are not composable (alas).

Anything like append_cmd_nospace ? or any other way?

I'm afraid there is no way to do this right now. One way to support
this is to extend the hook mechanism to work with actions:

(case (not_empty "-l"), (append_cmd "$CALL(AppendLibSuffix)"))

Sanjiv_Gupta · September 13, 2009, 7:14am

Mikhail Glushenkov wrote:

Hi,

Looks good.
One more quick query.
How to extract libname from "-l std" from the driver and pass it as
"std.lib" to the linker tool?
I know that unpack_values will give me "std", but an (append_cmd ".lib")
with that will insert a space.

This won't work since actions are not composable (alas).

Anything like append_cmd_nospace ? or any other way?

I'm afraid there is no way to do this right now. One way to support
this is to extend the hook mechanism to work with actions:

(case (not_empty "-l"), (append_cmd "$CALL(AppendLibSuffix)"))

Thanks for thinking over it.
On second thoughts, I feel that it isn't right to ask for a feature for every such thing. The right way to do this is to fix one's linker itself so that it accepts a -l option as such forwarded by the driver.

- Sanjiv

Mikhail_Glushenkov1 · September 15, 2009, 10:22pm

Hi,