[smoke] Replace deprecated -fopenmp-target-ignore-env-vars flag - #2053
[smoke] Replace deprecated -fopenmp-target-ignore-env-vars flag#2053nicebert wants to merge 1 commit into
Conversation
|
so these need to likely be moved to smok-dev and be landed and merged into aomp:amd-staging, |
|
please provide the correct llvm-project PR link. The current link goes to an AOMP PR. And yes, as Ron said already, these tests need to be moved to smoke-dev first. Support for the 2 over-subscription flags does not exist in older compilers. |
|
What is the deprecation path for ROCm? e.g. does it get added to docs and we wait for a release cycle? |
sorry, the link in the "depends on" worked correctly, the other one didn't. |
8fe34db to
3e158bf
Compare
I would not say that ignore-envars had no codegen implementation. It has a corresponding implementation. But it is being replaced with the over-subscription options to be in sync with upstream.
Xteam does not need this guarantee any more, that is legacy. No-Loop needs this guarantee. So please remove mention of Xteam from here.
|
3e158bf to
744fdb6
Compare
ronlieb
left a comment
There was a problem hiding this comment.
comment says to replace deprecated flag, but i do not see the flags being textually replaced, instead we are adding two flags to some tests ???
|
I talked to @ronlieb about the test move a few weeks ago, which turned out to not be required after all since the functionality is still present just not under the name of the flag |
|
w.r.t. the deprecation path: should we add a deprecation warning before removing it? then I need to create a new PR first that does this before we can move ahead with the two PRs removing the flag implementation and replacing it with the two upstream variants in the tests |
|
@ronlieb to answer your "change request" about the flag replacement: the flag was used in the implementation as a short cut for the other two flags, so to ensure functional equivalence we need to instead pass those |
But older compilers will fail with the new version of the tests, isn't it? That's because even if older compilers honor those 2 new options from upstream, they don't do anything based on those options. Only newer compilers actually implement anything for those 2 options. |
e35eaf7 to
76eb2b0
Compare
I can be more explicit in the message if you'd prefer. Tests that depend on the oversubscription behavior the deprecated flag used to gate got direct replacement. I removed it in tests that didn't require the flag in the first place. |
has this been addressed ? |
Update 27 tests (19 smoke, 8 smoke-limbo) to use replacement flags for specialized kernel generation. The -fopenmp-target-ignore-env-vars flag is being replaced with -fopenmp-assume-teams-oversubscription and -fopenmp-assume-threads-oversubscription to sync with upstream LLVM. The replacement flags provide equivalent functionality for no-loop kernel optimizations by indicating that the runtime can ignore environment variables for thread/team configuration.
76eb2b0 to
b14dc41
Compare
|
Squashed the two commits into a single commit and changed two more tests that weren't passing with "older compilers" with the replacement: target-fast still implies ignore-env-vars so negative flag variants did not lead to condition correctly evaluating to false for no-loop kernel gate, replaced target-fast with the explicit flags in those. |
| RUNCMD = ./$(TESTNAME) 2>&1 | $(FILECHECK) $(TESTSRC_MAIN) | ||
|
|
||
| CFLAGS += -fopenmp-target-fast -fno-openmp-target-ignore-env-vars | ||
| CFLAGS += -fopenmp-assume-no-thread-state -fopenmp-assume-no-nested-parallelism |
There was a problem hiding this comment.
This test is checking whether the no-variant works for the option. So now it should have
CFLAGS += -fopenmp-target-fast -fno-openmp-assume-teams-oversubscription -fno-openmp-assume-threads-oversubscription
These -no-options should disable no-loop generation and the test should pass.
| RUNCMD = ./$(TESTNAME) 2>&1 | $(FILECHECK) $(TESTSRC_MAIN) | ||
|
|
||
| CFLAGS += -O3 -fno-openmp-target-ignore-env-vars -fopenmp-target-fast | ||
| CFLAGS += -O3 -fopenmp-assume-no-thread-state -fopenmp-assume-no-nested-parallelism |
There was a problem hiding this comment.
same comment as no-loop4.
| TESTSRC_ALL = $(TESTSRC_MAIN) $(TESTSRC_AUX) | ||
|
|
||
| CFLAGS += -O3 -fno-openmp-target-ignore-env-vars | ||
| CFLAGS += -O3 |
There was a problem hiding this comment.
same comment as in no-loop-4/
| TESTSRC_ALL = $(TESTSRC_MAIN) $(TESTSRC_AUX) | ||
|
|
||
| CFLAGS += -O3 -fno-openmp-target-ignore-env-vars | ||
| CFLAGS += -O3 |
There was a problem hiding this comment.
same comment as in no-loop-4.
| TESTSRC_ALL = $(TESTSRC_MAIN) $(TESTSRC_AUX) | ||
|
|
||
| CFLAGS += -O3 -fno-openmp-target-ignore-env-vars | ||
| CFLAGS += -O3 |
| TESTSRC_ALL = $(TESTSRC_MAIN) $(TESTSRC_AUX) | ||
|
|
||
| CFLAGS += -O3 -fno-openmp-target-ignore-env-vars | ||
| CFLAGS += -O3 |
|
I realized that my suggestion for no-loop-4 won't work until ROCm/llvm-project#1698 lands. So there may be a chicken-and-egg situation between these 2 PRs. One possibility is to ignore my suggestions for this PR, make a mental note to add some coverage for the no-variants of the oversubscription options in the presence of -fopenmp-target-fast, and add that coverage back after the llvm-project PR#1698 lands. I will approve this PR assuming the above plan. |
dhruvachak
left a comment
There was a problem hiding this comment.
LGTM, remember to add coverage for the no-variants of the over-subscription options after ROCm/llvm-project#1698 lands.
[smoke] Replace deprecated -fopenmp-target-ignore-env-vars flag
Update 27 tests (19 smoke, 8 smoke-limbo) to use replacement flags
for specialized kernel generation. The -fopenmp-target-ignore-env-vars
flag is removed from ROCm/llvm-project#1698.
Replaced with -fopenmp-assume-teams-oversubscription and
-fopenmp-assume-threads-oversubscription, which provide equivalent
functionality by indicating that the runtime can ignore environment
variables for thread/team configuration, enabling no-loop kernel optimizations.
Depends on: ROCm/llvm-project#1698