Skip to content

[MachineLoopInfo] Fix assertion failure on undef use operands #100137

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 24, 2024
Merged

[MachineLoopInfo] Fix assertion failure on undef use operands #100137

merged 6 commits into from
Jul 24, 2024

Conversation

jayfoad
Copy link
Contributor

@jayfoad jayfoad commented Jul 23, 2024

Fixes #100115

@llvmbot
Copy link
Member

llvmbot commented Jul 23, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: Jay Foad (jayfoad)

Changes

Fixes #100115

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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/MachineLoopInfo.cpp (+1-1)
  • (modified) llvm/test/CodeGen/AMDGPU/dpp64_combine.ll (+23-2)
diff --git a/llvm/lib/CodeGen/MachineLoopInfo.cpp b/llvm/lib/CodeGen/MachineLoopInfo.cpp
index a03c008e6045a..88ba10fbe29a9 100644
--- a/llvm/lib/CodeGen/MachineLoopInfo.cpp
+++ b/llvm/lib/CodeGen/MachineLoopInfo.cpp
@@ -287,7 +287,7 @@ bool MachineLoop::isLoopInvariant(MachineInstr &I,
       }
     }
 
-    if (!MO.isUse())
+    if (!MO.readsReg())
       continue;
 
     assert(MRI->getVRegDef(Reg) &&
diff --git a/llvm/test/CodeGen/AMDGPU/dpp64_combine.ll b/llvm/test/CodeGen/AMDGPU/dpp64_combine.ll
index b4218bc2afc7f..3d8cda589f46f 100644
--- a/llvm/test/CodeGen/AMDGPU/dpp64_combine.ll
+++ b/llvm/test/CodeGen/AMDGPU/dpp64_combine.ll
@@ -1,7 +1,7 @@
 ; RUN: llc -mtriple=amdgcn -mcpu=gfx90a -verify-machineinstrs < %s | FileCheck %s -check-prefixes=GCN,DPP64,GFX90A
 ; RUN: llc -mtriple=amdgcn -mcpu=gfx940 -verify-machineinstrs < %s | FileCheck %s -check-prefixes=GCN,DPP64,DPPMOV64
-; RUN: llc -mtriple=amdgcn -mcpu=gfx1010 -verify-machineinstrs < %s | FileCheck %s -check-prefixes=GCN,DPP32,GFX10PLUS
-; RUN: llc -mtriple=amdgcn -mcpu=gfx1100 -verify-machineinstrs < %s | FileCheck %s -check-prefixes=GCN,DPP32,GFX10PLUS
+; RUN: llc -mtriple=amdgcn -mcpu=gfx1010 -verify-machineinstrs < %s | FileCheck %s -check-prefixes=GCN,DPP32,GFX10PLUS,GFX10
+; RUN: llc -mtriple=amdgcn -mcpu=gfx1100 -verify-machineinstrs < %s | FileCheck %s -check-prefixes=GCN,DPP32,GFX10PLUS,GFX11
 
 ; GCN-LABEL: {{^}}dpp64_ceil:
 ; GCN:           global_load_{{dwordx2|b64}} [[V:v\[[0-9:]+\]]],
@@ -69,6 +69,27 @@ define amdgpu_kernel void @dpp64_div(ptr addrspace(1) %arg, i64 %in1) {
   ret void
 }
 
+; GCN-LABEL: {{^}}dpp64_loop:
+; GCN: v_mov_b32_dpp
+; DPP64: v_mov_b32_dpp
+; GFX90A: v_add_co_u32_e32
+; GFX90A: v_addc_co_u32_e32
+; DPPMOV64: v_lshl_add_u64
+; GFX10: v_mov_b32_dpp
+; GFX10: v_add_co_u32
+; GFX10: v_add_co_ci_u32_e32
+; GFX11: v_add_co_u32_e64_dpp
+; GFX11: v_add_co_ci_u32_e32
+define amdgpu_cs void @dpp64_loop(i64 %arg) {
+bb:
+  br label %bb1
+bb1:
+  %i = call i64 @llvm.amdgcn.update.dpp.i64(i64 0, i64 0, i32 0, i32 0, i32 0, i1 false)
+  %i2 = add i64 %i, %arg
+  %i3 = atomicrmw add ptr addrspace(1) null, i64 %i2 monotonic, align 8
+  br label %bb1
+}
+
 declare i32 @llvm.amdgcn.workitem.id.x()
 declare i64 @llvm.amdgcn.update.dpp.i64(i64, i64, i32, i32, i32, i1) #0
 declare double @llvm.ceil.f64(double)

arsenm
arsenm reviewed Jul 23, 2024
View reviewed changes
llvm/lib/CodeGen/MachineLoopInfo.cpp
@@ -287,7 +287,7 @@ bool MachineLoop::isLoopInvariant(MachineInstr &I,
}
}

if (!MO.isUse())
if (!MO.readsReg())
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs a mir test too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

arsenm
arsenm approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

lgtm with nits

llvm/test/CodeGen/AMDGPU/machinelicm-crash.mir Outdated
Comment on lines 47 to 48
%59:vgpr_32 = IMPLICIT_DEF
%60:sreg_32 = IMPLICIT_DEF
Copy link
Contributor

Choose a reason for hiding this comment

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

run-pass=none to compact register numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is straight out of llvm-reduce in MIR mode. It would be really handy if llvm-reduce could do the compaction itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done anyway.

Copy link
Contributor

@arsenm arsenm Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yes, that was on my todo list last I was working on the mir support. But we also just have a lot of bad defaults in mir print/parse

llvm/test/CodeGen/AMDGPU/machinelicm-crash.mir Outdated
@@ -0,0 +1,77 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
# RUN: llc -mtriple=amdgcn -mcpu=gfx1100 -run-pass=early-machinelicm %s | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

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

More specific test file / function name, mention the issue number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jayfoad jayfoad merged commit b9995a1 into llvm:main Jul 24, 2024
7 checks passed
@jayfoad jayfoad deleted the update-64-licm branch July 24, 2024 13:27
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
@jayfoad @yuxuanchen1997
[MachineLoopInfo] Fix assertion failure on undef use operands (#100137)
28cc806 
Summary: Fixes #100115

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250741
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arsenm arsenm arsenm approved these changes

@pravinjagtap pravinjagtap pravinjagtap approved these changes

@sjoerdmeijer sjoerdmeijer Awaiting requested review from sjoerdmeijer

@vikramRH vikramRH Awaiting requested review from vikramRH

Assignees
No one assigned
Labels
backend:AMDGPU
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[AMDGPU] Assertion failure in MachineLICM when using llvm.amdgcn.update.dpp.i64
4 participants
@jayfoad @llvmbot @arsenm @pravinjagtap