[PATCH] D122569: [lit] Support %if ... %else syntax for RUN lines

Tue Mar 29 12:34:10 PDT 2022

asavonic added a comment.

In D122569#3412230 <https://reviews.llvm.org/D122569#3412230>, @delcypher wrote:

> Have you considered the different approach of having conditional `RUN` lines instead? E.g.
>
>   RUN: echo this always runs
>   RUN(feature): echo this only runs when feature is true
>   RUN(!feature): echo this only runs when feature is false
>   
>   RUN(feature || feature2): echo this only runs where feature or feature2 is true.
>   RUN(!(feature || feature2)): echo this only runs where feature and feature2 are both false
>
> I think this would make `RUN` lines easier to read. It doesn't support the "else" case quite as cleanly (you have to duplicate the condition and then negate it on separate line) but I think that's pretty minor because the condition can easily be made a "feature" that is computed from other features (in a test suite's `lit.cfg.py`)

Both options should work fine for ptxas or any other external tools.
`%if .. %else` syntax is more generic and it is actually easier to implement in LIT: it is just a bit more complicated substitution, whereas `RUN(cond)` is completely new syntax that (may) require more work to enable (at least I couldn't figure out how to do this quickly).

In D122569#3414112 <https://reviews.llvm.org/D122569#3414112>, @probinson wrote:

> I see that for `%if feature {A} %else {B}` the space before the `%else` is not preserved in the result.
> If there is no `%else` what happens? e.g., `XX %if feature {YY} ZZ`  Asking because I see no test case for this.

Spaces before %else are ignored only if %else is present. Added this test:

  # RUN: echo XX %if feature {YY} ZZ
  # RUN: echo AA %if feature {BB} %else {CC} DD
  # RUN: echo AA %if nofeature {BB} %else {CC} DD

  echo XX YY ZZ
  echo AA BB DD
  echo AA CC DD

> I take it the `%if` expressions take only lit feature keywords, like REQUIRES? No triple substrings, as allowed by UNSUPPORTED/XFAIL?
> (I have a back-burner task to make all three of those be consistent, this would be a fourth situation.)

Yes, identifiers are taken from `config.available_features`, just like for `REQUIRES`. The last patch adds binary expression evaluation, e.g. `feature1 && !feature2` or regex match `{{fea.+}}`.

> I actually agree with @delcypher that `RUN(feature):` seems cleaner and certainly handles the motivating case very well. But it is obviously less powerful, and I won't object to the `%if` solution.

Agree.

================
Comment at: llvm/docs/TestingGuide.rst:615

+``%if feature {<if branch>} %else {<else branch>}``
+
----------------
jhenderson wrote:
> jhenderson wrote:
> > tra wrote:
> > > delcypher wrote:
> > > > asavonic wrote:
> > > > > tra wrote:
> > > > > > Would any of the following be expected to work?
> > > > > > - `%if feature {do_something} | FileCheck %s`   -- this will probably fail as empty substitution would produce no output to check
> > > > > > - `%if feature {do_something | FileCheck %s}` -- this would probably work.
> > > > > > - `%if featureA { %if featureB {do_AB} %else {do_A_notB}} %else {do_notA}` -- I suspect we'll fail to parse it correctly.
> > > > > > 
> > > > > > I'd document that we currently can't nest those `%if/%else` and, maybe add the `%if feature {do_something | FileCheck %s}` as a canonical example of how to use it for conditional output-checking tests.
> > > > > > 
> > > > > > Would any of the following be expected to work?
> > > > > > - `%if feature {do_something} | FileCheck %s`   -- this will probably fail as empty substitution would produce no output to check
> > > > > 
> > > > > Right. If the feature is not available then we'll get ` | FileCheck %s`.
> > > > >  
> > > > > > - `%if feature {do_something | FileCheck %s}` -- this would probably work.
> > > > > 
> > > > > This does not work for verbose output: LIT formats a command into `echo "RUN at line #"; ${command};` and if `${command}` is an empty string we'll get `echo "RUN at line #"; ;`. Bash cannot parse the double semicolon at the end.
> > > > > 
> > > > > We can probably handle this as a special case.
> > > > > 
> > > > > > - `%if featureA { %if featureB {do_AB} %else {do_A_notB}} %else {do_notA}` -- I suspect we'll fail to parse it correctly.
> > > > > 
> > > > > Recursive substitution actually works if `config.recursiveExpansionLimit` is set in lit.cfg. I'll this case to the test.
> > > > > 
> > > > > > 
> > > > > > I'd document that we currently can't nest those `%if/%else` and, maybe add the `%if feature {do_something | FileCheck %s}` as a canonical example of how to use it for conditional output-checking tests.
> > > > > > 
> > > > > 
> > > > > As I mentioned above, a canonical example would be:
> > > > > `%if feature {do_something | FileCheck %s} %else {true}` 
> > > > > 
> > > > I am not a fan of this syntax.
> > > > 
> > > > 1. Being prefixed with `%` suggests it is substitution, but `%if` and `%else` are not normal substitutions. It might be worth introducing different syntax for these.
> > > > 2. Lit has this annoying problem with substitutions are prefixes of other substitutions (e.g. `%s` and `%source`) where the order that substitutions added matter. There's a possibility that by introducing `%if` and `%else` that this will break existing substitutions (e.g. `%if_something` and `%else_something`). You could avoid this by using something like `%if%` and `%else%`.
> > > > LIT formats a command into echo "RUN at line #"; ${command}; and if ${command} is an empty string we'll get echo "RUN at line #"; ;. Bash cannot parse the double semicolon at the end.
> > > 
> > > Perhaps we should expand the empty branch to `:` which *is* the standard null command in shell https://pubs.opengroup.org/onlinepubs/009695399/utilities/colon.html
> > > 
> > > Perhaps we should expand the empty branch to : which *is* the standard null command in shell https://pubs.opengroup.org/onlinepubs/009695399/utilities/colon.html
> > 
> > Don't forget Windows (where colons are not null actions).
> > 
> > > I am not a fan of this syntax.
> > > 
> > > 1. Being prefixed with `%` suggests it is substitution, but `%if` and `%else` are not normal substitutions. It might be worth introducing different syntax for these.
> > > 2. Lit has this annoying problem with substitutions are prefixes of other substitutions (e.g. `%s` and `%source`) where the order that substitutions added matter. There's a possibility that by introducing `%if` and `%else` that this will break existing substitutions (e.g. `%if_something` and `%else_something`). You could avoid this by using something like `%if%` and `%else%`.
> > 
> > I'm not convinced about adding more substitutions, especially `%if%` style since that's the canonical way of specifying environment variables in a Windows command prompt. %if/%else is pretty intuitive, I reckon. I personally don't think 2 is a real issue, unless there is already existing problems within the LLVM code-base like this. Yes, there might be downstream users, but a) we don't bend over backwards to avoid breaking them, and b) there's a clear path forward (rename the downstream substitution).
> > I am not a fan of this syntax.
> > 
> > 1. Being prefixed with `%` suggests it is substitution, but `%if` and `%else` are not normal substitutions. It might be worth introducing different syntax for these.
> > 2. Lit has this annoying problem with substitutions are prefixes of other substitutions (e.g. `%s` and `%source`) where the order that substitutions added matter. There's a possibility that by introducing `%if` and `%else` that this will break existing substitutions (e.g. `%if_something` and `%else_something`). You could avoid this by using something like `%if%` and `%else%`.
> 
> 
> > - `%if feature {do_something | FileCheck %s}` -- this would probably work.
> 
> This does not work for verbose output: LIT formats a command into `echo "RUN at line #"; ${command};` and if `${command}` is an empty string we'll get `echo "RUN at line #"; ;`. Bash cannot parse the double semicolon at the end.
> 
> We can probably handle this as a special case.

Fixed this by changing how this debug print is implemented. Now we don't print an extra semicolon if the command is empty.

================
Comment at: llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt:9
+
+# RUN: %if feature  \
+# RUN:   { echo     \
----------------
delcypher wrote:
> Does parsing more complicated expressions work? I would expect it to, so we should probably add tests for it.
> Does parsing more complicated expressions work? I would expect it to, so we should probably add tests for it.

This is now implemented in the latest revision of the patch.

================
Comment at: llvm/utils/lit/tests/shtest-if-else.py:1-2
+# RUN: %{lit} -v --show-all %{inputs}/shtest-if-else > %t.out
+# RUN: FileCheck --input-file %t.out %s
+# END.
----------------
tra wrote:
> Why not just pipe the output straight to FileCheck and avoid the temp file.
Done.

================
Comment at: llvm/utils/lit/tests/shtest-if-else.py:19
+# CHECK-NEXT: Exit Code: 0
+
----------------
jhenderson wrote:
> Nit: too many blank lines at EOF.
Fixed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122569