This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Make "-fno-split-machine-functions" a valid flag for all archs
ClosedPublic

Authored by shenhan on Aug 24 2023, 10:20 AM.

Details

Summary

Previously, when clang reports an error when -fno-split-machine-functions is used for non-X86 archs.

However, in some cases, users may specify flags as "-fsplit-machine-functions -fother-flags -fno-split-machine-functions", the first one is from a global flag set, the last one is used to negate the global flag, we think this is a valid usage mode.

Another cases is when clang is used to invoke multiple workloads, like "-x cuda -fsplit-machine-functions -Xarch_device -fno-split-machine-functions", the latter is used to negate the global -fsplit-machine-functions when invoke workloads for GPU."

This change makes this work, so all below are valid usages and does not have any warnings:

  • ./bin/clang -x cuda -nocudalib -nocudainc -Xarch_host -fsplit-machine-functions -S ~/test.c
  • ./bin/clang -x cuda -nocudalib -nocudainc -fsplit-machine-functions -Xarch_device -fnosplit-machine-functions -S ~/test.c
  • ./bin/clang -x cuda -nocudalib -nocudainc -Xarch_host -fsplit-machine-functions -Xarch_device -fnosplit-machine-functions -S ~/test.c

Diff Detail

Event Timeline

shenhan created this revision.Aug 24 2023, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2023, 10:20 AM
Herald added a subscriber: pengfei. · View Herald Transcript
shenhan requested review of this revision.Aug 24 2023, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2023, 10:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
shenhan edited the summary of this revision. (Show Details)Aug 24 2023, 10:22 AM
shenhan edited the summary of this revision. (Show Details)
shenhan edited the summary of this revision. (Show Details)
shenhan added subscribers: xur, tmsriram.

Thanks for the patch. We need a test. I cleaned up fsplit-machine-functions.c a bit. You can rebase and modify the not %clang -### -c --target=arm-unknown-linux -fsplit-machine-functions -fno-split-machine-functions RUN line in clang/test/Driver/fsplit-machine-function.c

shenhan updated this revision to Diff 553219.Aug 24 2023, 11:56 AM

Thanks for the patch. We need a test. I cleaned up fsplit-machine-functions.c a bit. You can rebase and modify the not %clang -### -c --target=arm-unknown-linux -fsplit-machine-functions -fno-split-machine-functions RUN line in clang/test/Driver/fsplit-machine-function.c

Thanks. Fixed the case tagged with "FIXME" and added some new cases.

MaskRay added inline comments.Aug 24 2023, 1:30 PM
clang/test/Driver/fsplit-machine-functions.c
19

We should not test clang: before error:. It may be a different string in certain builds where %clang is a symlink to a file not named clang. This would also fail on Windows. Rule of thumb: don't test the exact string before error:

21

We don't really need new tests. The existing --target=arm-unknown-linux one I added is sufficient.

shenhan updated this revision to Diff 553256.Aug 24 2023, 2:05 PM
shenhan marked 2 inline comments as done.
shenhan added inline comments.
clang/test/Driver/fsplit-machine-functions.c
19

Acked.

21

Done, removed newly added cases.

MaskRay accepted this revision.Aug 24 2023, 2:10 PM

With a test nit

clang/test/Driver/fsplit-machine-functions.c
19

Suggest that we add -Werror (%clang -### -Werror ...) to additionally test that there is no warning. -Werror upgrades warnings to errors, which would cause -### to exit with 1. This is a recent change of mine to make -### stricter.

This revision is now accepted and ready to land.Aug 24 2023, 2:10 PM

Make "-fno-split-machine-functions" a valid flag for all archs.

Driver changes usually are named [Driver] .... The popular convention is to omit the full stop at the end of the subject.

shenhan retitled this revision from Make "-fno-split-machine-functions" a valid flag for all archs. to [Driver] Make "-fno-split-machine-functions" a valid flag for all archs.Aug 24 2023, 2:31 PM
This revision was landed with ongoing or failed builds.Aug 24 2023, 3:56 PM
This revision was automatically updated to reflect the committed changes.