[PATCH] D83004: [UpdateCCTestChecks] Include generated functions if asked

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added a comment.

I think this approach seems fine, just a few very minor comments inlines.

In D83004#2280363 <https://reviews.llvm.org/D83004#2280363>, @greened wrote:

> In D83004#2278681 <https://reviews.llvm.org/D83004#2278681>, @arichardson wrote:
>
>> Maybe add another test that checks for the @__cxx_global_var_init() constructor function?
>> E.g. something like this:
>>
>>   int init_func(int arg) {
>>       return arg + 1;
>>   }
>>   
>>   int my_global = init_func(2);
>
> Can you explain what this would test beyond the provided OpenMP tests?  Remember, the tests are for the update tool itself, not to test that the compiler is doing the right thing.

It wouldn't add anything in terms of test coverage, I was just thinking of an example where the generated function is (at least currently) always inserted at the same location.



================
Comment at: llvm/utils/UpdateTestChecks/common.py:608
+def find_arg_in_test(test_info, finder, arg_string, warn):
+  result = finder(test_info.args)
+  if not result:
----------------
I found this function a bit difficult to parse due to not knowing what `finder` does. Maybe something like `get_arg_to_check` ?


================
Comment at: llvm/utils/update_cc_test_checks.py:294
+      def check_generator(my_output_lines, prefixes, func):
+        if '-emit-llvm' in clang_args:
+          common.add_ir_checks(my_output_lines, '//',
----------------
This is currently always true, but I guess adding it doesn't do any harm.


================
Comment at: llvm/utils/update_cc_test_checks.py:301
+        else:
+          asm.add_asm_checks(my_output_lines, ';',
+                             prefixes,
----------------
Since we're writing the output to a C/C++ file we need to use `//` comments.


================
Comment at: llvm/utils/update_cc_test_checks.py:331
+            # are ordered by prefix instead of by function as in "normal"
+            # mode.
+            if '-emit-llvm' in clang_args:
----------------
greened wrote:
> arichardson wrote:
> > jdoerfert wrote:
> > > This is all unfortunate but at least for OpenMP not easily changeable. Let's go with this.
> > How difficult would it be to only use this behaviour if the normal one fails? 
> > 
> > If we encounter a function that's not in the source code we just append the check lines for the generate function where we last added checks. And if we then see a function where the order doesn't match the source order fall back to just emitting all checks at the end of the file?
> > How difficult would it be to only use this behaviour if the normal one fails? 
> > 
> > If we encounter a function that's not in the source code we just append the check lines for the generate function where we last added checks. And if we then see a function where the order doesn't match the source order fall back to just emitting all checks at the end of the file?
> 
> The problem is that this loop is driven by lines as they appear in the source file and uses a dictionary to look up the tool output for that function.  Because of this checks will *always* be placed in the source before each function and there is no way to tell if the checks you're adding are from out-of-order tool output.  Changing this would be a complete rewrite of the algorithm, which I wanted to avoid for this enhancement.  Of course we can always rewrite it later but I think that is best done as a separate change.
> 
> The other two tools use a similar algorithm.  In fact it *may* be possible to further refactor this code to share the check output driver.  But again, that involves changing existing code and is best left to a separate change I think.
Yes, I think the current approach is fine since it will only affect new tests that add `--include-generated-funcs`. In the future I think it would be nice if this flag could be passed to existing tests that don't have any generated functions and keep the output stable (other than the UTC_ARGS).


================
Comment at: llvm/utils/update_llc_test_checks.py:14
 import os  # Used to advertise this file's name ("autogenerated_note").
+import sys
 
----------------
seems unused?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83004