This is an archive of the discontinued LLVM Phabricator instance.

Differential D41377

[AMDGPU] Turn off MergeConsecutiveStores() before Instruction Selection for AMDGPU
ClosedPublic

Authored by msearles on Dec 18 2017, 6:14 PM.

Details

Reviewers
arsenm
rampitec
Commits
rGe4f067ebe2f8: [AMDGPU] Turn off MergeConsecutiveStores() before Instruction Selection for…
rL321100: [AMDGPU] Turn off MergeConsecutiveStores() before Instruction Selection for…
Summary

Turn off MergeConsecutiveStores() before Instruction Selection for AMDGPU. Commit dbbb6c5fc3642987430866dffdf710df4f616ac7 turned on MergeConsecutiveStores() before Instruction Selection for all targets. Enough AMDGPU compiles go into an infinite loop ( MergeConsecutiveStores() merges two stores; LegalizeStoreOps() un-merges; MergeConsecutiveStores() re-merges, etc. ) to warrant turning it off until the issues can be addressed.

Diff Detail

Repository
rL LLVM

Event Timeline

msearles created this revision.Dec 18 2017, 6:14 PM
Herald added subscribers: t-tye, tpr, dstuttard and 4 others. · View Herald TranscriptDec 18 2017, 6:14 PM
whchung added a subscriber: whchung.Dec 18 2017, 7:08 PM
msearles updated this revision to Diff 127533.Dec 19 2017, 8:18 AM

Ran instnamer on memory-legalizer-store-infinite-loop.ll

msearles added a reviewer: rampitec.Dec 19 2017, 8:59 AM
rampitec added inline comments.Dec 19 2017, 9:30 AM
test/CodeGen/AMDGPU/memory-legalizer-store-infinite-loop.ll
1 ↗(On Diff #127533)

Please use -check-prefix=GCN instead of CHECK.

msearles updated this revision to Diff 127552.Dec 19 2017, 9:46 AM

Use -check-prefix=GCN instead of CHECK; add GCN-LABEL

msearles marked an inline comment as done.Dec 19 2017, 9:46 AM
arsenm added inline comments.Dec 19 2017, 10:00 AM
lib/Target/AMDGPU/AMDGPUISelLowering.h
205 ↗(On Diff #127552)

Needs a comment explaining why

msearles updated this revision to Diff 127558.Dec 19 2017, 10:19 AM

Add comment per reviewer request

msearles marked an inline comment as done.Dec 19 2017, 10:19 AM
arsenm accepted this revision.Dec 19 2017, 10:26 AM

LGTM with minor fixes

lib/Target/AMDGPU/AMDGPUISelLowering.h
206 ↗(On Diff #127558)

Should refer to the svn revision since that is still the canonical name

test/CodeGen/AMDGPU/memory-legalizer-store-infinite-loop.ll
14 ↗(On Diff #127558)

Check the stores

15 ↗(On Diff #127558)

You can remove weak_odr

This revision is now accepted and ready to land.Dec 19 2017, 10:26 AM
msearles updated this revision to Diff 127572.Dec 19 2017, 11:15 AM

Adjust per reviewer comments.

msearles marked 3 inline comments as done.Dec 19 2017, 11:15 AM
arsenm accepted this revision.Dec 19 2017, 11:17 AM

LGTM

lib/Target/AMDGPU/AMDGPUISelLowering.h
207–208 ↗(On Diff #127572)

Just r 319036, the rest is junk

Closed by commit rL321100: [AMDGPU] Turn off MergeConsecutiveStores() before Instruction Selection for… (authored by msearles). · Explain WhyDec 19 2017, 11:27 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
Target/
AMDGPU/
10 lines
test/
CodeGen/
AMDGPU/
3 lines
32 lines

Diff 127578

llvm/trunk/lib/Target/AMDGPU/AMDGPUISelLowering.h

Show First 20 Lines • Show All 196 Lines • ▼ Show 20 Lines void ReplaceNodeResults(SDNode * N,
SelectionDAG &DAG) const override; SelectionDAG &DAG) const override;
SDValue combineFMinMaxLegacy(const SDLoc &DL, EVT VT, SDValue LHS, SDValue combineFMinMaxLegacy(const SDLoc &DL, EVT VT, SDValue LHS,
SDValue RHS, SDValue True, SDValue False, SDValue RHS, SDValue True, SDValue False,
SDValue CC, DAGCombinerInfo &DCI) const; SDValue CC, DAGCombinerInfo &DCI) const;
const char* getTargetNodeName(unsigned Opcode) const override; const char* getTargetNodeName(unsigned Opcode) const override;
// FIXME: Turn off MergeConsecutiveStores() before Instruction Selection
// for AMDGPU.
// A commit ( git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@319036
// 91177308-0d34-0410-b5e6-96231b3b80d8 ) turned on
// MergeConsecutiveStores() before Instruction Selection for all targets.
// Enough AMDGPU compiles go into an infinite loop ( MergeConsecutiveStores()
// merges two stores; LegalizeStoreOps() un-merges; MergeConsecutiveStores()
// re-merges, etc. ) to warrant turning it off for now.
bool mergeStoresAfterLegalization() const override { return false; }
bool isFsqrtCheap(SDValue Operand, SelectionDAG &DAG) const override { bool isFsqrtCheap(SDValue Operand, SelectionDAG &DAG) const override {
return true; return true;
} }
SDValue getSqrtEstimate(SDValue Operand, SelectionDAG &DAG, int Enabled, SDValue getSqrtEstimate(SDValue Operand, SelectionDAG &DAG, int Enabled,
int &RefinementSteps, bool &UseOneConstNR, int &RefinementSteps, bool &UseOneConstNR,
bool Reciprocal) const override; bool Reciprocal) const override;
SDValue getRecipEstimate(SDValue Operand, SelectionDAG &DAG, int Enabled, SDValue getRecipEstimate(SDValue Operand, SelectionDAG &DAG, int Enabled,
int &RefinementSteps) const override; int &RefinementSteps) const override;
▲ Show 20 Lines • Show All 259 Lines • Show Last 20 Lines

llvm/trunk/test/CodeGen/AMDGPU/amdgpu.private-memory.ll

Show First 20 Lines • Show All 245 Lines • ▼ Show 20 Linesentry:
store i32 %5, i32 addrspace(1)* %out store i32 %5, i32 addrspace(1)* %out
ret void ret void
} }
; FUNC-LABEL: {{^}}char_array: ; FUNC-LABEL: {{^}}char_array:
; R600: MOVA_INT ; R600: MOVA_INT
; SI-PROMOTE-DAG: buffer_store_short v{{[0-9]+}}, off, s[{{[0-9]+:[0-9]+}}], s{{[0-9]+}} offset:4 ; encoding: ; SI-PROMOTE-DAG: buffer_store_byte v{{[0-9]+}}, off, s[{{[0-9]+:[0-9]+}}], s{{[0-9]+}} offset:4 ; encoding:
; SI-PROMOTE-DAG: buffer_store_byte v{{[0-9]+}}, off, s[{{[0-9]+:[0-9]+}}], s{{[0-9]+}} offset:5 ; encoding:
; SI-ALLOCA-DAG: buffer_store_byte v{{[0-9]+}}, off, s[{{[0-9]+:[0-9]+}}], s{{[0-9]+}} offset:4 ; encoding: [0x04,0x00,0x60,0xe0 ; SI-ALLOCA-DAG: buffer_store_byte v{{[0-9]+}}, off, s[{{[0-9]+:[0-9]+}}], s{{[0-9]+}} offset:4 ; encoding: [0x04,0x00,0x60,0xe0
; SI-ALLOCA-DAG: buffer_store_byte v{{[0-9]+}}, off, s[{{[0-9]+:[0-9]+}}], s{{[0-9]+}} offset:5 ; encoding: [0x05,0x00,0x60,0xe0 ; SI-ALLOCA-DAG: buffer_store_byte v{{[0-9]+}}, off, s[{{[0-9]+:[0-9]+}}], s{{[0-9]+}} offset:5 ; encoding: [0x05,0x00,0x60,0xe0
define amdgpu_kernel void @char_array(i32 addrspace(1)* %out, i32 %index) #0 { define amdgpu_kernel void @char_array(i32 addrspace(1)* %out, i32 %index) #0 {
entry: entry:
%0 = alloca [2 x i8] %0 = alloca [2 x i8]
%1 = getelementptr inbounds [2 x i8], [2 x i8]* %0, i32 0, i32 0 %1 = getelementptr inbounds [2 x i8], [2 x i8]* %0, i32 0, i32 0
%2 = getelementptr inbounds [2 x i8], [2 x i8]* %0, i32 0, i32 1 %2 = getelementptr inbounds [2 x i8], [2 x i8]* %0, i32 0, i32 1
▲ Show 20 Lines • Show All 300 Lines • Show Last 20 Lines

llvm/trunk/test/CodeGen/AMDGPU/memory-legalizer-store-infinite-loop.ll

; RUN: llc -mtriple=amdgcn--amdhsa-amdgiz -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
; Effectively, check that the compile finishes; in the case
; of an infinite loop, llc toggles between merging 2 ST4s
; ( MergeConsecutiveStores() ) and breaking the resulting ST8
; apart ( LegalizeStoreOps() ).
target datalayout = "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:32:32-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-A5"
; GCN-LABEL: {{^}}_Z6brokenPd:
; GCN: flat_store_dword v[{{[0-9]+}}:{{[0-9]+}}], {{v[0-9]+}}
; GCN: flat_store_dword v[{{[0-9]+}}:{{[0-9]+}}], {{v[0-9]+}}
define amdgpu_kernel void @_Z6brokenPd(double* %arg) {
bb:
%tmp = alloca double, align 8, addrspace(5)
%tmp1 = alloca double, align 8, addrspace(5)
%tmp2 = load double, double* %arg, align 8
br i1 1, label %bb6, label %bb4
bb3: ; No predecessors!
br label %bb4
bb4: ; preds = %bb3, %bb
%tmp5 = phi double addrspace(5)* [ %tmp1, %bb3 ], [ %tmp, %bb ]
store double %tmp2, double addrspace(5)* %tmp5, align 8
br label %bb6
bb6: ; preds = %bb4, %bb
%tmp7 = phi double [ 0x7FF8123000000000, %bb4 ], [ 0x7FF8000000000000, %bb ]
store double %tmp7, double* %arg, align 8
ret void
}