Skip to content

Commit 1896fb2

Browse files
committed
[SelectionDAG] Assume that a GlobalAlias may alias other global values
This fixes a bug detected in DAGCombiner when using global alias variables. Here is an example: @foo = global i16 0, align 1 @aliasFoo = alias i16, i16 * @foo define i16 @bar() { ... store i16 7, i16 * @foo, align 1 store i16 8, i16 * @aliasFoo, align 1 ... } BaseIndexOffset::computeAliasing would incorrectly derive NoAlias for the two accesses in the example above, resulting in DAGCombiner miscompiles. This patch fixes the problem by a defensive approach letting BaseIndexOffset::computeAliasing return false, i.e. that the aliasing couldn't be determined, when comparing two global values and at least one is a GlobalAlias. In the future we might improve this with a deeper analysis to look at the aliasee for the GlobalAlias etc. But that is a bit more complicated considering that we could have 'local_unnamed_addr' and situations with several 'alias' variables. Fixes PR51878. Differential Revision: https://reviews.llvm.org/D110064
1 parent d009f6e commit 1896fb2

File tree

3 files changed

+93
-7
lines changed

3 files changed

+93
-7
lines changed

llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "llvm/CodeGen/SelectionDAG.h"
1515
#include "llvm/CodeGen/SelectionDAGNodes.h"
1616
#include "llvm/CodeGen/TargetLowering.h"
17+
#include "llvm/IR/GlobalAlias.h"
1718
#include "llvm/Support/Casting.h"
1819
#include "llvm/Support/Debug.h"
1920
#include <cstdint>
@@ -143,13 +144,31 @@ bool BaseIndexOffset::computeAliasing(const SDNode *Op0,
143144
bool IsCV0 = isa<ConstantPoolSDNode>(BasePtr0.getBase());
144145
bool IsCV1 = isa<ConstantPoolSDNode>(BasePtr1.getBase());
145146

146-
// If of mismatched base types or checkable indices we can check
147-
// they do not alias.
148-
if ((BasePtr0.getIndex() == BasePtr1.getIndex() || (IsFI0 != IsFI1) ||
149-
(IsGV0 != IsGV1) || (IsCV0 != IsCV1)) &&
150-
(IsFI0 || IsGV0 || IsCV0) && (IsFI1 || IsGV1 || IsCV1)) {
151-
IsAlias = false;
152-
return true;
147+
if ((IsFI0 || IsGV0 || IsCV0) && (IsFI1 || IsGV1 || IsCV1)) {
148+
// We can derive NoAlias In case of mismatched base types.
149+
if (IsFI0 != IsFI1 || IsGV0 != IsGV1 || IsCV0 != IsCV1) {
150+
IsAlias = false;
151+
return true;
152+
}
153+
// We cannot safely determine whether the pointers alias if we compare two
154+
// global values and at least one is a GlobalAlias.
155+
if (IsGV0 && IsGV1 &&
156+
(isa<GlobalAlias>(
157+
cast<GlobalAddressSDNode>(BasePtr0.getBase())->getGlobal()) ||
158+
isa<GlobalAlias>(
159+
cast<GlobalAddressSDNode>(BasePtr1.getBase())->getGlobal())))
160+
return false;
161+
// If checkable indices we can check they do not alias.
162+
// FIXME: Please describe this a bit better. Looks weird to say that there
163+
// is no alias if the indices are the same. Is this code assuming that
164+
// someone has checked that the base isn't the same as a precondition? And
165+
// what about offsets? And what about NumBytes0 and NumBytest1 (can we
166+
// really derive NoAlias here if we do not even know how many bytes that are
167+
// dereferenced)?
168+
if (BasePtr0.getIndex() == BasePtr1.getIndex()) {
169+
IsAlias = false;
170+
return true;
171+
}
153172
}
154173
return false; // Cannot determine whether the pointers alias.
155174
}
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
2+
; RUN: llc -O1 -mtriple i686-unknown-linux-gnu -o - %s | FileCheck %s
3+
4+
@foo = global i16 0, align 1
5+
@aliasFoo = alias i16, i16 * @foo
6+
@bar = global i16 0, align 1
7+
8+
; This used to miscompile due to not realizing that the store to @aliasFoo
9+
; clobbered @foo (see PR51878).
10+
;
11+
; With some improvements to codegen it should be possible to detect that @foo
12+
; and @aliasFoo are aliases, and that @aliasFoo isn't aliasing with @bar. So
13+
; ideally we would end up with three movw instructions here. Running opt
14+
; before llc on this test case would take care of that, but llc is not smart
15+
; enough to deduce that itself yet.
16+
define i16 @main() {
17+
; CHECK-LABEL: main:
18+
; CHECK: # %bb.0: # %entry
19+
; CHECK-NEXT: movw $1, foo
20+
; CHECK-NEXT: movw $2, bar
21+
; CHECK-NEXT: movw $4, aliasFoo
22+
; CHECK-NEXT: movzwl foo, %eax
23+
; CHECK-NEXT: addw bar, %ax
24+
; CHECK-NEXT: retl
25+
entry:
26+
store i16 1, i16 * @foo
27+
store i16 2, i16 * @bar
28+
store i16 4, i16 * @aliasFoo
29+
%foo = load i16, i16 * @foo
30+
%bar = load i16, i16 * @bar
31+
%res = add i16 %foo, %bar
32+
ret i16 %res
33+
}

llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ class SelectionDAGAddressAnalysisTest : public testing::Test {
3030

3131
void SetUp() override {
3232
StringRef Assembly = "@g = global i32 0\n"
33+
"@g_alias = alias i32, i32* @g\n"
3334
"define i32 @f() {\n"
3435
" %1 = load i32, i32* @g\n"
3536
" ret i32 %1\n"
@@ -63,6 +64,9 @@ class SelectionDAGAddressAnalysisTest : public testing::Test {
6364
G = M->getGlobalVariable("g");
6465
if (!G)
6566
report_fatal_error("G?");
67+
AliasedG = M->getNamedAlias("g_alias");
68+
if (!AliasedG)
69+
report_fatal_error("AliasedG?");
6670

6771
MachineModuleInfo MMI(TM.get());
6872

@@ -89,6 +93,7 @@ class SelectionDAGAddressAnalysisTest : public testing::Test {
8993
std::unique_ptr<Module> M;
9094
Function *F;
9195
GlobalVariable *G;
96+
GlobalAlias *AliasedG;
9297
std::unique_ptr<MachineFunction> MF;
9398
std::unique_ptr<SelectionDAG> DAG;
9499
};
@@ -209,6 +214,35 @@ TEST_F(SelectionDAGAddressAnalysisTest, globalWithFrameObject) {
209214
EXPECT_FALSE(IsAlias);
210215
}
211216

217+
TEST_F(SelectionDAGAddressAnalysisTest, globalWithAliasedGlobal) {
218+
SDLoc Loc;
219+
220+
EVT GTy = DAG->getTargetLoweringInfo().getValueType(DAG->getDataLayout(),
221+
G->getType());
222+
SDValue GValue = DAG->getConstant(0, Loc, GTy);
223+
SDValue GAddr = DAG->getGlobalAddress(G, Loc, GTy);
224+
SDValue GStore = DAG->getStore(DAG->getEntryNode(), Loc, GValue, GAddr,
225+
MachinePointerInfo(G, 0));
226+
Optional<int64_t> GNumBytes = MemoryLocation::getSizeOrUnknown(
227+
cast<StoreSDNode>(GStore)->getMemoryVT().getStoreSize());
228+
229+
SDValue AliasedGValue = DAG->getConstant(1, Loc, GTy);
230+
SDValue AliasedGAddr = DAG->getGlobalAddress(AliasedG, Loc, GTy);
231+
SDValue AliasedGStore =
232+
DAG->getStore(DAG->getEntryNode(), Loc, AliasedGValue, AliasedGAddr,
233+
MachinePointerInfo(AliasedG, 0));
234+
235+
bool IsAlias;
236+
bool IsValid = BaseIndexOffset::computeAliasing(GStore.getNode(), GNumBytes,
237+
AliasedGStore.getNode(),
238+
GNumBytes, *DAG, IsAlias);
239+
240+
// With some deeper analysis we could detect if G and AliasedG is aliasing or
241+
// not. But computeAliasing is currently defensive and assumes that a
242+
// GlobalAlias might alias with any global variable.
243+
EXPECT_FALSE(IsValid);
244+
}
245+
212246
TEST_F(SelectionDAGAddressAnalysisTest, fixedSizeFrameObjectsWithinDiff) {
213247
SDLoc Loc;
214248
auto Int8VT = EVT::getIntegerVT(Context, 8);

0 commit comments

Comments
 (0)