Skip to content

Commit 230f453

Browse files
author
Simon Dardis
committedNov 24, 2017
[CodeGenPrepare] Check that erased sunken address are not reused
CodeGenPrepare sinks address computations from one basic block to another and attempts to reuse address computations that have already been sunk. If the same address computation appears twice with the first instance as an operand of a load whose result is an operand to a simplifable select, CodeGenPrepare simplifies the select and recursively erases the now dead instructions. CodeGenPrepare then attempts to use the erased address computation for the second load. Fix this by erasing the cached address value if it has zero uses before looking for the address value in the sunken address map. This partially resolves PR35209. Thanks to Alexander Richardson for reporting the issue! This fixed version relands r318032 which was reverted in r318049 due to sanitizer buildbot failures. Reviewers: john.brawn Differential Revision: https://reviews.llvm.org/D39841 llvm-svn: 318956
·
llvmorg-21-initllvmorg-6.0.0-rc1
1 parent 0e8924a commit 230f453

File tree

3 files changed

+80
-5
lines changed

3 files changed

+80
-5
lines changed
 

‎llvm/lib/CodeGen/CodeGenPrepare.cpp

+14-5
Original file line numberDiff line numberDiff line change
@@ -245,8 +245,10 @@ class TypePromotionTransaction;
245245

246246
/// Keeps track of non-local addresses that have been sunk into a block.
247247
/// This allows us to avoid inserting duplicate code for blocks with
248-
/// multiple load/stores of the same address.
249-
ValueMap<Value*, Value*> SunkAddrs;
248+
/// multiple load/stores of the same address. The usage of WeakTrackingVH
249+
/// enables SunkAddrs to be treated as a cache whose entries can be
250+
/// invalidated if a sunken address computation has been erased.
251+
ValueMap<Value*, WeakTrackingVH> SunkAddrs;
250252

251253
/// Keeps track of all instructions inserted for the current function.
252254
SetOfInstrs InsertedInsts;
@@ -4436,9 +4438,13 @@ bool CodeGenPrepare::optimizeMemoryInst(Instruction *MemoryInst, Value *Addr,
44364438

44374439
// Now that we determined the addressing expression we want to use and know
44384440
// that we have to sink it into this block. Check to see if we have already
4439-
// done this for some other load/store instr in this block. If so, reuse the
4440-
// computation.
4441-
Value *&SunkAddr = SunkAddrs[Addr];
4441+
// done this for some other load/store instr in this block. If so, reuse
4442+
// the computation. Before attempting reuse, check if the address is valid
4443+
// as it may have been erased.
4444+
4445+
WeakTrackingVH SunkAddrVH = SunkAddrs[Addr];
4446+
4447+
Value * SunkAddr = SunkAddrVH.pointsToAliveValue() ? SunkAddrVH : nullptr;
44424448
if (SunkAddr) {
44434449
DEBUG(dbgs() << "CGP: Reusing nonlocal addrmode: " << AddrMode << " for "
44444450
<< *MemoryInst << "\n");
@@ -4663,6 +4669,9 @@ bool CodeGenPrepare::optimizeMemoryInst(Instruction *MemoryInst, Value *Addr,
46634669
}
46644670

46654671
MemoryInst->replaceUsesOfWith(Repl, SunkAddr);
4672+
// Store the newly computed address into the cache. In the case we reused a
4673+
// value, this should be idempotent.
4674+
SunkAddrs[Addr] = WeakTrackingVH(SunkAddr);
46664675

46674676
// If we have no uses, recursively delete the value and all dead instructions
46684677
// using it.

‎llvm/test/Transforms/CodeGenPrepare/Mips/lit.local.cfg

+2
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
if not 'Mips' in config.root.targets:
2+
config.unsupported = True

‎llvm/test/Transforms/CodeGenPrepare/Mips/pr35209.ll

+64
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
; RUN: opt -S -mtriple=mips64-mti-linux-gnu -codegenprepare < %s | FileCheck %s
2+
3+
; Test that if an address that was sunk from a dominating bb, used in a
4+
; select that is erased along with its' trivally dead operand, that the
5+
; sunken address is not reused if the same address computation occurs
6+
; after the select. Previously, this caused a ICE.
7+
8+
%struct.az = type { i32, %struct.bt* }
9+
%struct.bt = type { i32 }
10+
%struct.f = type { %struct.ax, %union.anon }
11+
%struct.ax = type { %struct.az* }
12+
%union.anon = type { %struct.bd }
13+
%struct.bd = type { i64 }
14+
%struct.bg = type { i32, i32 }
15+
%struct.ap = type { i32, i32 }
16+
17+
@ch = common global %struct.f zeroinitializer, align 8
18+
@j = common global %struct.az* null, align 8
19+
@ck = common global i32 0, align 4
20+
@h = common global i32 0, align 4
21+
@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
22+
23+
define internal void @probestart() {
24+
entry:
25+
%0 = load %struct.az*, %struct.az** @j, align 8
26+
%bw = getelementptr inbounds %struct.az, %struct.az* %0, i64 0, i32 1
27+
%1 = load i32, i32* @h, align 4
28+
%cond = icmp eq i32 %1, 0
29+
br i1 %cond, label %sw.bb, label %cl
30+
31+
sw.bb: ; preds = %entry
32+
%call = tail call inreg { i64, i64 } @ba(i32* bitcast (%struct.f* @ch to i32*))
33+
br label %cl
34+
35+
cl: ; preds = %sw.bb, %entry
36+
%2 = load %struct.bt*, %struct.bt** %bw, align 8
37+
%tobool = icmp eq %struct.bt* %2, null
38+
%3 = load i32, i32* @ck, align 4
39+
%.sink5 = select i1 %tobool, i32* getelementptr (%struct.bg, %struct.bg* bitcast (%union.anon* getelementptr inbounds (%struct.f, %struct.f* @ch, i64 0, i32 1) to %struct.bg*), i64 0, i32 1), i32* getelementptr (%struct.ap, %struct.ap* bitcast (%union.anon* getelementptr inbounds (%struct.f, %struct.f* @ch, i64 0, i32 1) to %struct.ap*), i64 0, i32 1)
40+
store i32 %3, i32* %.sink5, align 4
41+
store i32 1, i32* bitcast (i64* getelementptr inbounds (%struct.f, %struct.f* @ch, i64 0, i32 1, i32 0, i32 0) to i32*), align 8
42+
%4 = load %struct.bt*, %struct.bt** %bw, align 8
43+
tail call void (i8*, ...) @a(i8* getelementptr inbounds ([1 x i8], [1 x i8]* @.str, i64 0, i64 0), %struct.bt* %4)
44+
ret void
45+
}
46+
47+
; CHECK-LABEL: @probestart()
48+
; CHECK-LABEL: entry:
49+
; CHECK: %[[I0:[0-9]+]] = load %struct.az*, %struct.az** @j
50+
; CHECK-LABEL: cl:
51+
52+
; CHECK-NOT: %{{[0-9]+}} = load %struct.bt*, %struct.bt** %bw
53+
; CHECK-NOT: %{{[.a-z0-9]}} = select
54+
; CHECK-NOT: %{{[0-9]+}} = load %struct.bt*, %struct.bt** %bw
55+
56+
; CHECK: %[[I1:[0-9]+]] = bitcast %struct.az* %[[I0]] to i8*
57+
; CHECK-NEXT: %sunkaddr = getelementptr i8, i8* %[[I1]], i64 8
58+
; CHECK-NEXT: %[[I2:[0-9]+]] = bitcast i8* %sunkaddr to %struct.bt**
59+
; CHECK-NEXT: %{{[0-9]+}} = load %struct.bt*, %struct.bt** %[[I2]]
60+
; CHECK-NEXT: tail call void (i8*, ...) @a
61+
62+
declare inreg { i64, i64 } @ba(i32*)
63+
64+
declare void @a(i8*, ...)

0 commit comments

Comments
 (0)
Please sign in to comment.