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

RegisterPressure: Uncomment out assert #87405

Closed
wants to merge 1 commit into from

Conversation

arsenm
Copy link
Contributor

@arsenm arsenm commented Apr 2, 2024

This appears to have been accidentally commented out in 91b5cf8

This appears to have been accidentally commented out in
91b5cf8
@arsenm
Copy link
Contributor Author

arsenm commented Apr 2, 2024

This fails in quite a few AMDGPU tests

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 3, 2024

@llvm/pr-subscribers-llvm-regalloc

Author: Matt Arsenault (arsenm)

Changes

This appears to have been accidentally commented out in 91b5cf8


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/RegisterPressure.cpp (+1-1)
diff --git a/llvm/lib/CodeGen/RegisterPressure.cpp b/llvm/lib/CodeGen/RegisterPressure.cpp
index f86aa3a167202f..9320abd8963b12 100644
--- a/llvm/lib/CodeGen/RegisterPressure.cpp
+++ b/llvm/lib/CodeGen/RegisterPressure.cpp
@@ -64,7 +64,7 @@ static void increaseSetPressure(std::vector<unsigned> &CurrSetPressure,
 static void decreaseSetPressure(std::vector<unsigned> &CurrSetPressure,
                                 const MachineRegisterInfo &MRI, Register Reg,
                                 LaneBitmask PrevMask, LaneBitmask NewMask) {
-  //assert((NewMask & !PrevMask) == 0 && "Must not add bits");
+  assert((NewMask & ~PrevMask).none() && "Must not add bits");
   if (NewMask.any() || PrevMask.none())
     return;
 

@@ -64,7 +64,7 @@ static void increaseSetPressure(std::vector<unsigned> &CurrSetPressure,
static void decreaseSetPressure(std::vector<unsigned> &CurrSetPressure,
const MachineRegisterInfo &MRI, Register Reg,
LaneBitmask PrevMask, LaneBitmask NewMask) {
//assert((NewMask & !PrevMask) == 0 && "Must not add bits");
assert((NewMask & ~PrevMask).none() && "Must not add bits");
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 783 should most likely be PreviousMask |= LiveOut. When I do that, the following tests fail in "check-llvm-codegen":

Failed Tests (21):
  LLVM :: CodeGen/AMDGPU/amdgpu.private-memory.ll
  LLVM :: CodeGen/AMDGPU/coalesce-vgpr-alignment.ll
  LLVM :: CodeGen/AMDGPU/dag-divergence-atomic.ll
  LLVM :: CodeGen/AMDGPU/global_atomics.ll
  LLVM :: CodeGen/AMDGPU/global_atomics_i64.ll
  LLVM :: CodeGen/AMDGPU/insert_vector_dynelt.ll
  LLVM :: CodeGen/AMDGPU/itofp.i128.ll
  LLVM :: CodeGen/AMDGPU/lds-misaligned-bug.ll
  LLVM :: CodeGen/AMDGPU/llvm.amdgcn.raw.buffer.atomic.ll
  LLVM :: CodeGen/AMDGPU/llvm.amdgcn.raw.ptr.buffer.atomic.ll
  LLVM :: CodeGen/AMDGPU/llvm.amdgcn.struct.buffer.atomic.ll
  LLVM :: CodeGen/AMDGPU/llvm.amdgcn.struct.buffer.load.format.ll
  LLVM :: CodeGen/AMDGPU/llvm.amdgcn.struct.ptr.buffer.atomic.ll
  LLVM :: CodeGen/AMDGPU/llvm.amdgcn.struct.ptr.buffer.load.format.ll
  LLVM :: CodeGen/AMDGPU/load-global-i32.ll
  LLVM :: CodeGen/AMDGPU/load-local-i32.ll
  LLVM :: CodeGen/AMDGPU/merge-stores.ll
  LLVM :: CodeGen/AMDGPU/promote-alloca-globals.ll
  LLVM :: CodeGen/AMDGPU/sgpr-copy.ll
  LLVM :: CodeGen/AMDGPU/spill-agpr.ll
  LLVM :: CodeGen/AMDGPU/v_mac.ll

At least one test fails because UseMask has more bits set in it than LiveLanes in RegPressureTracker::bumpUpwardPressure. I think there is some issue with how the lane masks are calculated/adjusted.

The tests I used is

llc -mtriple=amdgcn -mcpu=tonga -amdgpu-atomic-optimizer-strategy=None -mattr=-flat-for-global -verify-machineinstrs llvm/test/CodeGen/AMDGPU/global_atomics.ll -o -

@kparzysz
Copy link
Contributor

It wasn't accidental, tests were failing with that assertion, even back then.

@kparzysz
Copy link
Contributor

I think there is some issue in the register operand collection/liveness adjustment in cases where the same virtual register is used and defined in an instruction. Still investigating.

@arsenm
Copy link
Contributor Author

arsenm commented Apr 16, 2024

Replaced by #88892

@arsenm arsenm closed this Apr 16, 2024
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.

None yet

3 participants