-
Notifications
You must be signed in to change notification settings - Fork 608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Warn when --iree-llvmcpu-target-cpu defaults to "generic". #18682
Conversation
9d56ed2
to
cacd566
Compare
nit about GitHub: Please put tags like these in comments, not commit messages (or PR descriptions that then become commit messages). If someone pushes a commit containing such a tag to their fork, it will notify those users. I don't really need to know whenever someone rebases or otherwise rewrites history in a fork :P |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I like the refactoring into the helper file and methods.
We should think through the diagnostics and error handling a bit more, but beyond that I like the new code behavior.
cacd566
to
3ee9494
Compare
@ScottTodd , @benvanik , @stellaraccident , I just pushed what I have. It addresses all of @ScottTodd 's earlier review comments, and produces the expected results in I don't know how to fix this. @benvanik suggested |
After chatting some more with @ScottTodd , pushed a 2nd commit that makes it work AFAICS: 18748f7 WDYT? |
That seems ok to me. |
tests/e2e/regression/BUILD.bazel
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you resolve the merge conflicts on this file (and the CMakeLists.txt), so the github workflows run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! No major concerns on my end with this approach.
if(_RULE_TARGET_CPU) | ||
list(APPEND _BASE_COMPILER_FLAGS "--iree-llvmcpu-target-cpu=${_RULE_TARGET_CPU}") | ||
endif() | ||
if(_RULE_TARGET_CPU_FEATURES) | ||
list(APPEND _BASE_COMPILER_FLAGS "--iree-llvmcpu-target-cpu-features=${_RULE_TARGET_CPU_FEATURES}") | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for these getting their own flags instead of using COMPILER_FLAGS
?
In general, I want these build system functions to do less, not more - especially for global flags controlling target backend/device-specific options.
I don't really mind being this explicit in our tests:
TARGET_BACKEND
"llvm-cpu"
COMPILER_FLAGS
"--iree-llvmcpu-target-cpu=generic"
DRIVER
"local-task"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I just made that change in all related rules.
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
18748f7
to
30d54bf
Compare
@ScottTodd , this feels ready for review now. I have followed your suggestion to drop The As part of this CL, I had to add |
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Ah. That's on my list to remove too: https://github.com/iree-org/iree-test-suites/blob/3a0ea13cbdc954365d653a48cc99cc63a9ff09b3/linalg_ops/matmul/generate_e2e_matmul_tests.py#L529-L531
Also for code review, please try to avoid force pushing. I don't see a way to view the diff between when I last reviewed and the latest code >_> |
@ScottTodd , for my education - when my PR has a conflict with |
|
TIL, thanks. Because we ultimately rebase-and-squash when merging PRs, I had followed that workflow also locally. It hadn't occurred to me that I could create merge commits locally and on my PR branch, as that would still ultimately get rebased-and-squashed on merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. Just a few lingering comments.
if(_RULE_COMPILER_TARGET_BACKEND STREQUAL "llvm-cpu") | ||
list(APPEND _TRANSLATE_FLAGS "--iree-llvmcpu-target-cpu=generic") | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to COMPILER_FLAGS
at the call sites instead of adding a branch to the common function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
if(_RULE_TARGET_CPU_FEATURES) | ||
list(APPEND _GENERATOR_STANDARD_FLAGS "--requirements=${_RULE_TARGET_CPU_FEATURES}") | ||
endif() | ||
foreach(_COMPILER_FLAG IN LISTS _RULE_COMPILER_FLAGS) | ||
set(_CPU_FEATURES_REGEX "^--iree-llvmcpu-target-cpu-features=") | ||
if (_COMPILER_FLAG MATCHES "${_CPU_FEATURES_REGEX}") | ||
string(REGEX REPLACE "${_CPU_FEATURES_REGEX}" "" _CPU_FEATURES "${_COMPILER_FLAG}") | ||
list(APPEND _GENERATOR_STANDARD_FLAGS "--requirements=${_CPU_FEATURES}") | ||
endif() | ||
endforeach() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for now / in this repo.
I have followed your suggestion to drop
TARGET_CPU
andTARGET_CPU_FEATURES
. While doing do, I found the place which had originally motivated them: this is where we pass a--requirements
flag to the test runner so that it can check at runtime that the host CPU supports the expected features. Now that there isn't a separateTARGET_CPU_FEATURES
field anymore, we resort to parsing theCOMPILER_FLAGS
, which is not too bad and is a tiny detail, so it is the right call to deal with it rather than have thoseTARGET_CPU
andTARGET_CPU_FEATURES
flags everywhere.Ah. That's on my list to remove too: https://github.com/iree-org/iree-test-suites/blob/3a0ea13cbdc954365d653a48cc99cc63a9ff09b3/linalg_ops/matmul/generate_e2e_matmul_tests.py#L529-L531
# TODO(scotttodd): drop this and whatever logic in the test tool used it # multiple backends should be able to use the same input IR, so the # input IR shouldn't need things like CPU features in it
(Over in iree-test-suites) I think I also want to push this logic to the leaves, similar to how we have if(IREE_HIP_TEST_TARGET_CHIP)
controlling defining and running tests for the ROCm backend + HIP driver, we can check explicit options like IREE_TEST_SUITES_INCLUDE_ARM_SME_TESTS
or check automatically/programmatically for support.
Basically, each set of tests should make it obvious how the tests are enabled. The existing logic, which you are updating here, hides some of that decision making in helper functions. For remote targets like Android devices and local targets like GPUs, we can't as easily enumerate devices and supported features at configure time, and I don't want to special case any backend in the helper functions.
flags = [ | ||
"--iree-hal-target-backends=%s" % target_backend, | ||
"--iree-llvmcpu-target-cpu=generic", | ||
] + compiler_flags + input_type_flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this implicit flag needed in the Bazel helper function? It looks like you already updated several call sites to pass it explicitly. We shouldn't always set a llvmcpu
, even for tests using other targets.
I don't care as much about the Bazel side of that though, and it looks like CMake has the behavior that I want already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. There was something to change here, but to match what we do in the CMake implementation, it had to be something else. The top-level rules like iree_check_test_suite
and iree_generated_e2e_runner_test
take the special target_cpu_features_variants
parameter. This defaults to ["generic"]
on CMake, so it should also default to that on Bazel. So the "generic"
stays implicit but moves one level up within the rules.
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
@stellaraccident , i'm blissfully unaware of release schedules. If there is any upcoming release, this one is worth having on it. |
Build failures on MSVC: https://github.com/iree-org/iree/actions/runs/11401187470/job/31723569010#step:7:7793
See the pinned issue: #18432 |
|
@bjacob I added a bullet to the rolling release notes there:
When we next cut a stable release, the release notes on that issue will be included. Feel free to edit the issue if you have other phrasing you want us to use. |
Fixes these error logs: https://github.com/iree-org/iree/actions/runs/11407589945/job/31743932060#step:8:2241 Follow-up to #18682, which changed these target names.
Progress on #18561. This introduces a warning (which we intend to promote to an error in the future) when targeting a generic CPU without explicitly asking for it. This addresses a performance footgun as that IREE default results in low performance.
Along the way this grew into a substantial change to e2e testing rules:
TARGET_CPU
andTARGET_CPU_FEATURES
arguments are gone (were redundant withCOMPILER_FLAGS
).TARGET_CPU_FEATURES_VARIANTS
, the special value"default"
is renamed to"generic"
and a new value"host"
is also supported.Example warning (this is customized to the target architecture, here x86):