This is an archive of the discontinued LLVM Phabricator instance.

[clang][test] Fix clang machine-function-split tests
Needs RevisionPublic

Authored by shenhan on Aug 17 2023, 4:20 PM.

Details

Summary

This CL includes two changes:

  1. move clang backend-warnings test cases from Driver/ to CodeGen/.
  2. removed cd "$(dirname "%t")" (since the tests are now moved to CodeGen, and they no longer generates multiple output files, so we can simply use -o %t.)

Diff Detail

Event Timeline

shenhan created this revision.Aug 17 2023, 4:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2023, 4:20 PM
shenhan requested review of this revision.Aug 17 2023, 4:20 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 17 2023, 4:20 PM
MaskRay added inline comments.Aug 17 2023, 4:40 PM
clang/test/CodeGen/fsplit-machine-functions.c
42

-target has been deprecated since Clang 3.4. Use --target=

The warning test should be moved to Driver/.
Whenever a warning can be placed either in Driver or in Frontend, we prefer Driver.

clang/test/Driver/fsplit-machine-functions.c
4–5

if OPT1 is identical OPT2, we should use the same prefix.

If you want to test -fprofile-use=, test the extra CC1 option from the driver -fprofile-use=.

llvm/test/CodeGen/Generic/machine-function-splitter.ll
19

It's usually better to test that there is no warning at all, instead of just testing that there is no one specific warning.

shenhan updated this revision to Diff 551332.Aug 17 2023, 5:18 PM
shenhan marked an inline comment as done.
shenhan added inline comments.
clang/test/CodeGen/fsplit-machine-functions.c
42

-target has been deprecated since Clang 3.4. Use --target=

Replaced with --target=arm-unknown-linux-gnueabi

Whenever a warning can be placed either in Driver or in Frontend, we prefer Driver.

Just a little bit confused, you mentioned in the other comment that Driver tests should not run the backend. Most driver commands should use -###.
And the warning to be tested here is issued from backend, so -### does not trigger it at all, so I moved the tests from Driver/ to here . Do you suggest move these back to Driver/?

clang/test/Driver/fsplit-machine-functions.c
4–5

After a second thought, test of "fprofile-use" is irrelevant to fsplit-machine-functions flag, removed the test lines.

MaskRay added inline comments.Aug 17 2023, 6:52 PM
clang/test/CodeGen/fsplit-machine-functions.c
42

test/Driver tests %clang -### and driver options.

Other directories (e.g. test/CodeGen) test %clang_cc1 and CC1 options. %clang is discouraged.

shenhan updated this revision to Diff 551583.Aug 18 2023, 11:27 AM
shenhan edited the summary of this revision. (Show Details)
shenhan marked an inline comment as done.
shenhan added inline comments.
clang/test/CodeGen/fsplit-machine-functions.c
42

Thanks. Now test/CodeGen/ tests are using %clang_cc1 and test/Driver/ tests are using %clang -###.

MaskRay added inline comments.Aug 18 2023, 12:45 PM
clang/test/CodeGen/fsplit-machine-functions.c
2

Any reason system-linux is needed?

3

We usually place x86 tests under CodeGen/X86 and arm tests under CodeGen/ARM

clang/test/Driver/fsplit-machine-functions.c
1–2

If you only use -###, // REQUIRES: x86-registered-target is unneeded.

3–4

Use --target= for new tests.

llvm/test/CodeGen/Generic/machine-function-splitter.ll
19

Delete ; MFS_ON-NOT: warning: -fsplit-machine-functions is not valid for. It's unneeded given ; MFS_ON-NOT: warning:

shenhan updated this revision to Diff 551630.Aug 18 2023, 1:52 PM
shenhan marked 3 inline comments as done.
MaskRay accepted this revision.Aug 18 2023, 1:54 PM
This revision is now accepted and ready to land.Aug 18 2023, 1:54 PM
shenhan updated this revision to Diff 551632.Aug 18 2023, 1:55 PM
shenhan added inline comments.
clang/test/CodeGen/fsplit-machine-functions.c
2

Deleted. Also deleted "REQUIRES: system-linux" from "fsplit-machine-functions-with-cuda-nvptx.c".

3

I can moved the X86 cases to clang/test/CodeGen/X86, but there is no clang/test/CodeGen/Arm or similar, shall I create "clang/test/CodeGen/Arm" instead?

clang/test/Driver/fsplit-machine-functions.c
1–2

Ah, yes. Deleted.

shenhan updated this revision to Diff 551634.Aug 18 2023, 2:12 PM
shenhan marked 2 inline comments as done.
shenhan updated this revision to Diff 551680.Aug 18 2023, 5:37 PM
MaskRay resigned from this revision.Aug 19 2023, 2:52 PM

Sorry, after reading through D157750, I think the patch should be reverted.

clang/test/CodeGen/fsplit-machine-functions.c
3

Sorry, this is not llvm/test/CodeGen. For clang/test/CodeGen this is fine.

This revision now requires review to proceed.Aug 19 2023, 2:52 PM
MaskRay requested changes to this revision.Aug 19 2023, 2:53 PM
This revision now requires changes to proceed.Aug 19 2023, 2:53 PM