Page MenuHomePhabricator

[UpdateTestChecks] Allow $ in function names
ClosedPublic

Authored by greened on Jul 1 2020, 1:55 PM.

Details

Summary

Some compilers generation functions with '$' in their names, so recognize those
functions.

This also requires recognizing function names inside quotes in some contexts in
order to escape certain characters.

Diff Detail

Event Timeline

greened created this revision.Jul 1 2020, 1:55 PM
spatel added a comment.Jul 2 2020, 2:41 PM

It would be good to check in a test example alongside this change, so we know it works. (And we'll know what we are losing if this has to be reverted for some reason.)

llvm/utils/UpdateTestChecks/asm.py
124

Do these ARM regexes not need the extra '?' like the others?

It would be good to check in a test example alongside this change, so we know it works. (And we'll know what we are losing if this has to be reverted for some reason.)

Will do. I was just recently pointed to where tests live.

greened marked an inline comment as done.Jul 6 2020, 7:02 AM
greened added inline comments.
llvm/utils/UpdateTestChecks/asm.py
124

I'm not entirely certain. <func> will match " because [^:] matches ". I originally developed this against x86_64 and the way the asm printer works for that target, the function symbol itself is printed without quotes ($ is fine in symnbol names) but the *comment* following the label includes the quotes. I did the same for every other target though I should verify that is correct (I will add tests for every target). Since these AArch64 patterns don't include a comment after the labels I didn't have anywhere to put quotes.

We need a test.

If it helps, OpenMP begin/end declare variant mangles the names with $, so do we for CUDA (sometimes).

We need a test.

If it helps, OpenMP begin/end declare variant mangles the names with $, so do we for CUDA (sometimes).

Thanks. Where should tests go? I don't think they should go in clang's test/utils/update_cc_test_checks as this isn't just testing that tool. This adds capability to update_test_checks.py and update_llc_test_checks.py too.

We need a test.

If it helps, OpenMP begin/end declare variant mangles the names with $, so do we for CUDA (sometimes).

Thanks. Where should tests go? I don't think they should go in clang's test/utils/update_cc_test_checks as this isn't just testing that tool. This adds capability to update_test_checks.py and update_llc_test_checks.py too.

My bad, llvm/test/tools/UpdateTestChecks is the test folder for these things.

@greened Any luck with a test case?

@greened Any luck with a test case?

Working on it now. Got distracted with various other fires.

greened updated this revision to Diff 292217.Sep 16 2020, 7:41 AM

Added tests.

jdoerfert accepted this revision.Sep 16 2020, 8:06 AM

LGTM. Thx for the many tests.

This revision is now accepted and ready to land.Sep 16 2020, 8:06 AM
This revision was landed with ongoing or failed builds.Sep 16 2020, 12:36 PM
This revision was automatically updated to reflect the committed changes.

There exist downstream ARM compilers that doesn't support ARM64 and fail with Inputs/arm_function_name.ll. You should consider guarding arm64 specific tests.

There exist downstream ARM compilers that doesn't support ARM64 and fail with Inputs/arm_function_name.ll. You should consider guarding arm64 specific tests.

arm64 is an alias for aarch64, that should just be armv7 like the rest. Also the aarch64 test should probably gain an IOS test, it's weird that the arm one has one but not the aarch64 one.