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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
