This is an archive of the discontinued LLVM Phabricator instance.

Differential D102283

AMDGPU: Fix assert when rewriting saddr d16 loads
ClosedPublic

Authored by arsenm on May 11 2021, 3:55 PM.

Details

Reviewers
rampitec
Summary

moveOperands does not handle moving tied operands since it would
generally have to fixup the tied operand references. Avoid the assert
by untying and retying after the modification. These in place
modifications really aren't managable.

Diff Detail

Unit TestsFailed

TimeTest
240 msx64 debian > LLVM.ExecutionEngine/JITLink/X86::ELF_ehframe_basic.s
210 msx64 debian > LLVM.ExecutionEngine/JITLink/X86::ELF_skip_debug_sections.s
190 msx64 debian > LLVM.ExecutionEngine/JITLink/X86::MachO_skip_debug_sections.s
150 msx64 windows > LLVM.ExecutionEngine/JITLink/X86::ELF_skip_debug_sections.s
130 msx64 windows > LLVM.ExecutionEngine/JITLink/X86::MachO_skip_debug_sections.s

Event Timeline

arsenm created this revision.May 11 2021, 3:55 PM
Herald added subscribers: kerbowa, hiraditya, t-tye and 6 others. · View Herald TranscriptMay 11 2021, 3:55 PM
arsenm requested review of this revision.May 11 2021, 3:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 3:55 PM
Herald added a subscriber: wdng. · View Herald Transcript
rampitec accepted this revision.May 11 2021, 3:57 PM

LGTM

This revision is now accepted and ready to land.May 11 2021, 3:57 PM
Harbormaster completed remote builds in B103871: Diff 344581.May 11 2021, 4:29 PM
arsenm closed this revision.May 14 2021, 10:24 AM

c7cff08f79f775d70f0241f4671d66586a9e17a7

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
17 lines
test/
CodeGen/
AMDGPU/
24 lines

Diff 344581

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 5,060 Lines • ▼ Show 20 Lines if (OldVAddrIdx == NewVAddrIdx) {
Inst.RemoveOperand(OldSAddrIdx); Inst.RemoveOperand(OldSAddrIdx);
// Update the use list with the pointer we have just moved from vaddr to // Update the use list with the pointer we have just moved from vaddr to
// saddr poisition. Otherwise new vaddr will be missing from the use list. // saddr poisition. Otherwise new vaddr will be missing from the use list.
MRI.removeRegOperandFromUseList(&NewVAddr); MRI.removeRegOperandFromUseList(&NewVAddr);
MRI.addRegOperandToUseList(&NewVAddr); MRI.addRegOperandToUseList(&NewVAddr);
} else { } else {
assert(OldSAddrIdx == NewVAddrIdx); assert(OldSAddrIdx == NewVAddrIdx);
if (OldVAddrIdx >= 0) if (OldVAddrIdx >= 0) {
int NewVDstIn = AMDGPU::getNamedOperandIdx(NewOpc, AMDGPU::OpName::vdst_in);
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-      int NewVDstIn = AMDGPU::getNamedOperandIdx(NewOpc, AMDGPU::OpName::vdst_in);
+      int NewVDstIn =
+          AMDGPU::getNamedOperandIdx(NewOpc, AMDGPU::OpName::vdst_in);
Lint: Pre-merge checks: clang-format: please reformat the code ``` - int NewVDstIn = AMDGPU::getNamedOperandIdx…
// RemoveOperand doesn't try to fixup tied operand indexes at it goes, so
// it asserts. Untie the operands for now and retie them afterwards.
if (NewVDstIn != -1) {
int OldVDstIn = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst_in);
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-        int OldVDstIn = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst_in);
+        int OldVDstIn =
+            AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst_in);
Lint: Pre-merge checks: clang-format: please reformat the code ``` - int OldVDstIn = AMDGPU::getNamedOperandIdx…
Inst.untieRegOperand(OldVDstIn);
}
Inst.RemoveOperand(OldVAddrIdx); Inst.RemoveOperand(OldVAddrIdx);
if (NewVDstIn != -1) {
int NewVDst = AMDGPU::getNamedOperandIdx(NewOpc, AMDGPU::OpName::vdst);
Inst.tieOperands(NewVDst, NewVDstIn);
}
}
} }
if (VAddrDef && MRI.use_nodbg_empty(VAddrDef->getOperand(0).getReg())) if (VAddrDef && MRI.use_nodbg_empty(VAddrDef->getOperand(0).getReg()))
VAddrDef->eraseFromParent(); VAddrDef->eraseFromParent();
return true; return true;
} }
▲ Show 20 Lines • Show All 2,765 Lines • Show Last 20 Lines

llvm/test/CodeGen/AMDGPU/global-load-saddr-to-vaddr.ll

Show All 25 Linesbb3: ; preds = %bb3, %bb
%i = phi i32 [ %i2, %bb ], [ %i8, %bb3 ] %i = phi i32 [ %i2, %bb ], [ %i8, %bb3 ]
%i4 = zext i32 %i to i64 %i4 = zext i32 %i to i64
%i5 = getelementptr inbounds i32, i32 addrspace(1)* %arg, i64 %i4 %i5 = getelementptr inbounds i32, i32 addrspace(1)* %arg, i64 %i4
%i6 = load volatile i32, i32 addrspace(1)* %i5, align 4 %i6 = load volatile i32, i32 addrspace(1)* %i5, align 4
%i8 = add nuw nsw i32 %i, 1 %i8 = add nuw nsw i32 %i, 1
%i9 = icmp eq i32 %i8, 256 %i9 = icmp eq i32 %i8, 256
br i1 %i9, label %bb2, label %bb3 br i1 %i9, label %bb2, label %bb3
} }
; GCN-LABEL: {{^}}test_move_load_address_to_vgpr_d16_hi:
; GCN-NOT: v_readfirstlane_b32
; GCN: global_load_short_d16_hi v{{[0-9]+}}, v[{{[0-9:]+}}], off glc
define amdgpu_kernel void @test_move_load_address_to_vgpr_d16_hi(i16 addrspace(1)* nocapture %arg) {
bb:
%i1 = getelementptr inbounds i16, i16 addrspace(1)* %arg, i64 0
%load.pre = load volatile i16, i16 addrspace(1)* %i1, align 4
%i2 = zext i16 %load.pre to i32
br label %bb3
bb2: ; preds = %bb3
ret void
bb3: ; preds = %bb3, %bb
%i = phi i32 [ %i2, %bb ], [ %i8, %bb3 ]
%i4 = zext i32 %i to i64
%i5 = getelementptr inbounds i16, i16 addrspace(1)* %arg, i64 %i4
%i6 = load volatile i16, i16 addrspace(1)* %i5, align 4
%insertelt = insertelement <2 x i16> undef, i16 %i6, i32 1
%i8 = bitcast <2 x i16> %insertelt to i32
%i9 = icmp eq i32 %i8, 256
br i1 %i9, label %bb2, label %bb3
}