-
Notifications
You must be signed in to change notification settings - Fork 15.6k
[clang] [Sema] Check argument range for prefetchi* intrinsics #149745
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
Conversation
|
@llvm/pr-subscribers-clang Author: Timothy Herchen (anematode) ChangesFixes #144857 . I can create a test if desired, but I think the fix is trivial enough. <img width="805" height="105" alt="image" src="https://github.com/user-attachments/assets/aaee8e5f-6e65-4f04-b8b9-e4ae1434d958" /> Full diff: https://github.com/llvm/llvm-project/pull/149745.diff 1 Files Affected:
diff --git a/clang/lib/Sema/SemaX86.cpp b/clang/lib/Sema/SemaX86.cpp
index 5c149bdec7073..6bb3558972126 100644
--- a/clang/lib/Sema/SemaX86.cpp
+++ b/clang/lib/Sema/SemaX86.cpp
@@ -954,6 +954,11 @@ bool SemaX86::CheckBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
l = 0;
u = 15;
break;
+ case X86::BI__builtin_ia32_prefetchi:
+ i = 1;
+ l = 2; // _MM_HINT_T1
+ u = 3; // _MM_HINT_T0
+ break;
}
// Note that we don't force a hard error on the range check here, allowing
|
|
@llvm/pr-subscribers-backend-x86 Author: Timothy Herchen (anematode) ChangesFixes #144857 . I can create a test if desired, but I think the fix is trivial enough. <img width="805" height="105" alt="image" src="https://github.com/user-attachments/assets/aaee8e5f-6e65-4f04-b8b9-e4ae1434d958" /> Full diff: https://github.com/llvm/llvm-project/pull/149745.diff 1 Files Affected:
diff --git a/clang/lib/Sema/SemaX86.cpp b/clang/lib/Sema/SemaX86.cpp
index 5c149bdec7073..6bb3558972126 100644
--- a/clang/lib/Sema/SemaX86.cpp
+++ b/clang/lib/Sema/SemaX86.cpp
@@ -954,6 +954,11 @@ bool SemaX86::CheckBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
l = 0;
u = 15;
break;
+ case X86::BI__builtin_ia32_prefetchi:
+ i = 1;
+ l = 2; // _MM_HINT_T1
+ u = 3; // _MM_HINT_T0
+ break;
}
// Note that we don't force a hard error on the range check here, allowing
|
|
Thanks for the patch! You can add a test case for it like |
|
done :) |
76a7a50 to
8cfa0f9
Compare
phoebewang
left a comment
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.
LGTM.
|
hmmmm why did it fail |
phoebewang
left a comment
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.
Please check test failures.
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.
The test needs to be in a seperate file.
8cfa0f9 to
8f27a35
Compare
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.
Remove
8f27a35 to
d6b0822
Compare
|
Sorry about that, should work now... |
d6b0822 to
fd1bba6
Compare
shafik
left a comment
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 requires a release note, especially because it fixes a bug.
| #include <immintrin.h> | ||
|
|
||
| void test_invalid_prefetchi(void* p) { | ||
| __builtin_ia32_prefetchi(p, 1); // expected-error {{argument value 1 is outside the valid range [2, 3]}} |
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.
We have a lower and upper range, testing should validate both. Tests should be as complete, especially when it is apparent and to prevent regression in behavior in the future due to new changes.
We don't change the intrinsic behavior and bug fixes usually don't need a release note. It's only needed when backporting to release branch. |
…49745) Fixes llvm#144857 . I can create a test if desired, but I think the fix is trivial enough. <img width="805" height="105" alt="image" src="https://github.com/user-attachments/assets/aaee8e5f-6e65-4f04-b8b9-e4ae1434d958" />
Fixes #144857 . I can create a test if desired, but I think the fix is trivial enough.