Skip to content
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

GlobalISel: Guard return in llvm::getIConstantSplatVal #71989

Merged
merged 8 commits into from
Nov 14, 2023
Merged

GlobalISel: Guard return in llvm::getIConstantSplatVal #71989

merged 8 commits into from
Nov 14, 2023

Conversation

changpeng
Copy link
Contributor

getIConstantVRegValWithLookThrough could return NULL.

  Some upcoming intrinsics use these new types
  Some upcoming intrinsics use these new types
  It could be possible for getIConstantVRegValWithLookThrough to
return std::nullopt.
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 10, 2023

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-llvm-globalisel

Author: Changpeng Fang (changpeng)

Changes

getIConstantVRegValWithLookThrough could return NULL.


Full diff: https://github.com/llvm/llvm-project/pull/71989.diff

1 Files Affected:

  • (modified) llvm/lib/CodeGen/GlobalISel/Utils.cpp (+3-3)
diff --git a/llvm/lib/CodeGen/GlobalISel/Utils.cpp b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
index 473c3f452f8b1d9..eaf829f562b2dc9 100644
--- a/llvm/lib/CodeGen/GlobalISel/Utils.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
@@ -1116,9 +1116,9 @@ std::optional<APInt>
 llvm::getIConstantSplatVal(const Register Reg, const MachineRegisterInfo &MRI) {
   if (auto SplatValAndReg =
           getAnyConstantSplat(Reg, MRI, /* AllowUndef */ false)) {
-    std::optional<ValueAndVReg> ValAndVReg =
-        getIConstantVRegValWithLookThrough(SplatValAndReg->VReg, MRI);
-    return ValAndVReg->Value;
+    if (std::optional<ValueAndVReg> ValAndVReg =
+        getIConstantVRegValWithLookThrough(SplatValAndReg->VReg, MRI))
+      return ValAndVReg->Value;
   }
 
   return std::nullopt;

Copy link

github-actions bot commented Nov 10, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff d2fd1106f6879c410b6a807133090866e6c3a243 2155fe1374ead3bc4a1db6f4856ab1d0cf2392ff -- llvm/lib/CodeGen/GlobalISel/Utils.cpp
View the diff from clang-format here.
diff --git a/llvm/lib/CodeGen/GlobalISel/Utils.cpp b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
index eaf829f562..e4a69a1600 100644
--- a/llvm/lib/CodeGen/GlobalISel/Utils.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
@@ -1117,7 +1117,7 @@ llvm::getIConstantSplatVal(const Register Reg, const MachineRegisterInfo &MRI) {
   if (auto SplatValAndReg =
           getAnyConstantSplat(Reg, MRI, /* AllowUndef */ false)) {
     if (std::optional<ValueAndVReg> ValAndVReg =
-        getIConstantVRegValWithLookThrough(SplatValAndReg->VReg, MRI))
+            getIConstantVRegValWithLookThrough(SplatValAndReg->VReg, MRI))
       return ValAndVReg->Value;
   }
 

@rampitec
Copy link
Collaborator

Any tests?

@changpeng
Copy link
Contributor Author

Any tests?
Encountered this issue during a downstream branch testing. No test for trunk yet but think the issue should be here.

@jayfoad
Copy link
Contributor

jayfoad commented Nov 13, 2023

Typo in subject "Guard return ..."?

@changpeng changpeng changed the title GlobalISel: Guide return in llvm::getIConstantSplatVal GlobalISel: Guard return in llvm::getIConstantSplatVal Nov 14, 2023
@changpeng
Copy link
Contributor Author

Typo in subject "Guard return ..."?

You are right. Thanks.

  It could be possible for getIConstantVRegValWithLookThrough to
return std::nullopt.
@changpeng changpeng closed this Nov 14, 2023
@changpeng changpeng reopened this Nov 14, 2023
@changpeng changpeng merged commit 011c9ee into llvm:main Nov 14, 2023
3 of 6 checks passed
std::optional<ValueAndVReg> ValAndVReg =
getIConstantVRegValWithLookThrough(SplatValAndReg->VReg, MRI);
return ValAndVReg->Value;
if (std::optional<ValueAndVReg> ValAndVReg =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@changpeng
A comment here would be nice.
E.g., "Even if the value is a splat constant, it may not be an integer one."

zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
getIConstantVRegValWithLookThrough could return NULL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants