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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D122569#3458804 <https://reviews.llvm.org/D122569#3458804>, @asavonic wrote:

> In D122569#3458476 <https://reviews.llvm.org/D122569#3458476>, @arichardson wrote:
>
>> While this is not a huge amount of code to be added to lit, I do wonder if we really need it. Wouldn't something like `%feature{foo}` that expands to 0 or 1 be sufficient? You could then use shell conditionals/`test` to avoid running lines.
>> Looking at the examples from D121727 <https://reviews.llvm.org/D121727>, you could do the following:
>>
>>   RUN: if [ %feature{ptxas} -ne 0 ]; then ptxas -c %t-nvptx.ptx -o /dev/null; fi
>>   RUN: test %feature{ptxas} -ne 0 && ptxas -c %t-nvptx.ptx -o /dev/null
>
> The variant with `if` is probably not going to work on windows.

Agreed.  It won't work in lit's internal shell currently either.  `%if` is a portable `if`.  Maybe it would be good to point this out in the documentation?

> As for the `test` - I'm not sure how it works in this case.

My hunch is that `test` is also not portable.



================
Comment at: llvm/utils/lit/lit/TestRunner.py:1235
+            if not match:
+                raise ValueError("'%{' is missing for %if substitution")
+            cond = ln[:match.start()]
----------------
It would be nice to extend the test suite to check that these syntax errors are actually detected.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1306
+
+        while len(ln):
+            # %if starts a new scope ('if' branch). Push '%if cond' to
----------------
asavonic wrote:
> jdenny wrote:
> > Instead of a loop with a custom stack data structure, a recursive descent parser might be easier to understand and extend, especially if we add more constructs similar to `%if` later.  That is, the python call stack would be used as the parser stack, and stack entries would be simple local variables, function parameters, and function returns.
> > 
> > This is just a suggestion.  I'm not insisting.  However, I do invite other reviewers to offer their opinion.
> Done. It does look better as a recursive function.
Nice.  Thanks.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1363
             # seems reasonable.
             ln = _caching_re_compile(a).sub(str(b), escape(ln))
 
----------------
asavonic wrote:
> jdenny wrote:
> > Why does `%%` need to be escaped again here?
> This is because of recursive substitution:
> ```
> substitutions = [
>   ("%rec1, "%%s"),
>   ("%s", "/path")
> ]
> ```
> If we expand `%rec1` to `%%s` in the first iteration of the loop, we have to escape `%%` before the next iteration. Otherwise instead of of `%s` we'll get `%/path`. This is tested in `shtest-recursive-substitution/escaping` test.
Make sense.  Thanks for explaining.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt:22
+# RUN: echo test-%if feature %{ 3 %} %else %{ fail %}-test
+# RUN: echo test-%if feature %{ 4 4 %} %else %{ fail %}-test
+# CHECK-NEXT: {{^.*'RUN}}: at line [[#@LINE-2]]'; echo test- 3 -test
----------------
Maybe test the preservation of space in `%else` as well.


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt:75
+#
+# RUN: %if feature {echo %{%}}
+# CHECK-NEXT: {{^.*'RUN}}: at line [[#@LINE-1]]'; echo {}
----------------
asavonic wrote:
> jdenny wrote:
> > If we're going to have these `%{` and `%}` escapes, they need to be documented somewhere.  However, I think they're going to be confusing:
> > 
> > - First, `%{foobar}` is already a common syntax for lit substitutions.  The if-then-else syntax's `%{` and `}` collide with it.  For example, how should I write `%{pathsep}` in a then or else block?  I have to write it as `%%{pathsep%}`, right?
> > - More generally, whether `{` and `}` need to be escaped and whether `%{` and `%}` are defined depend on the context.  That seems error-prone.
> > 
> > What if we go the other way?  That is, we could change the `{` and `}` around a then or else block to `%{` and `%}`.  That way, the entire if-then-else syntax is clearly at the preprocessor/substitution level: `%if`, `%else`, `%{`, and `%}`.  On the other hand, `{` and `}` behave as usual in the shell code, and the only character in shell code that ever needs to be escaped continues to be `%`.
> > 
> > But does that syntax have collisions?
> > 
> > - I just tried `git grep 'RUN.*%}'` and found no matches in llvm's repo.  It seems `%}` is unambiguous.  If a use arises, it can be escaped as `%%}`.  Again, only `%` ever has to be escaped.
> > - The `%{` is really just for symmetry with `%}`.  The `%{` doesn't actually need to be recognized in arbitrary shell code anywhere.  That is, the if-then-else parser would only need to recognize it after an `%if cond` or `%else`.  There doesn't appear to be a way for it to collide with, for example, `%{pathsep}`.
> > 
> > Did I miss anything?
> Done. The patch now uses `%{` and `%}` as begin/end tokens. It also helps to distinguish them from boolean expressions with a regex-match (`{{regex}}`).
Thanks.  Please add a test that a substitution like `%{foo}` works in the then and else blocks.  You'll probably have to define your own as I'm not sure it's safe to rely on the way the built-in substitutions expand.


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

https://reviews.llvm.org/D122569