Skip to content

[AMDGPU] Correct assert that incorrectly chained multiple == operators. #70291

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

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Oct 26, 2023

I believe this assert was trying to check that 3 variables were equal to 0.

I think it instead got interpreted as ((DSWCount == DSWWithPermCount) == DSWWithSharedVMEMCount) == 0 I guess (DSWCount == DSWWithPermCount) was true because both counts were 0. Then true got compared to DSWWithSharedVMEMCount, and since DSWWithSharedVMEMCount is 0, that compare was false. And then that false compared equal to the final 0.

I believe this assert was trying to check that 3 variables were
equal to 0.

I think it instead got interpreted as ((DSWCount == DSWWithPermCount) == DSWWithSharedVMEMCount) == 0
I guess (DSWCount == DSWWithPermCount) was true because both counts
were 0. Then true got compared to DSWWithSharedVMEMCount, and since
DSWWithSharedVMEMCount is 0, that compare was false. And then that
false compared equal to the final 0.
@llvmbot
Copy link
Member

llvmbot commented Oct 26, 2023

@llvm/pr-subscribers-backend-amdgpu

Author: Craig Topper (topperc)

Changes

I believe this assert was trying to check that 3 variables were equal to 0.

I think it instead got interpreted as ((DSWCount == DSWWithPermCount) == DSWWithSharedVMEMCount) == 0 I guess (DSWCount == DSWWithPermCount) was true because both counts were 0. Then true got compared to DSWWithSharedVMEMCount, and since DSWWithSharedVMEMCount is 0, that compare was false. And then that false compared equal to the final 0.


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

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp (+2-2)
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp b/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp
index c67a21c639fc029..0b2bb98738be2aa 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp
@@ -1121,8 +1121,8 @@ void MFMASmallGemmSingleWaveOpt::applyIGLPStrategy(
   unsigned MFMACount = 0;
   unsigned DSRCount = 0;
 
-  assert((IsPostRA ||
-          DSWCount == DSWWithPermCount == DSWWithSharedVMEMCount == 0) &&
+  assert((IsPostRA || (DSWCount == 0 && DSWWithPermCount == 0 &&
+                       DSWWithSharedVMEMCount == 0)) &&
          "DSWCounters should be zero in pre-RA scheduling!");
   SmallVector<SUnit *, 6> DSWithPerms;
   for (auto &SU : DAG->SUnits) {

@topperc topperc merged commit 35baff8 into llvm:main Oct 26, 2023
@topperc topperc deleted the pr/amdgpu-assert branch October 26, 2023 15:02
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Feb 29, 2024
…s. (llvm#70291)

I believe this assert was trying to check that 3 variables were equal to
0.

I think it instead got interpreted as ((DSWCount == DSWWithPermCount) ==
DSWWithSharedVMEMCount) == 0 I guess (DSWCount == DSWWithPermCount) was
true because both counts were 0. Then true got compared to
DSWWithSharedVMEMCount, and since DSWWithSharedVMEMCount is 0, that
compare was false. And then that false compared equal to the final 0.

Change-Id: I8c33f7fd42198af9fa6a54f1dc48c5ace13d6c3a
rocm-ci pushed a commit to ROCm/llvm-project that referenced this pull request Apr 16, 2024
…s. (llvm#70291)

Change-Id: I19593c4b93f4f3e1820e692a52d26e7e8866c5cf
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.

3 participants