Skip to content

Commit

Permalink
[BasicAA] Fix -basicaa-recphi for geps with negative offsets
Browse files Browse the repository at this point in the history 
As shown in D82998, the basic-aa-recphi option can cause miscompiles for
gep's with negative constants. The option checks for recursive phi, that
recurse through a contant gep. If it finds one, it performs aliasing
calculations using the other phi operands with an unknown size, to
specify that an unknown number of elements after the initial value are
potentially accessed. This works fine expect where the constant is
negative, as the size is still considered to be positive. So this patch
expands the check to make sure that the constant is also positive.

Differential Revision: https://reviews.llvm.org/D83576

(cherry picked from commit 311fafd)
  • Loading branch information
@davemgreen @zmodem
davemgreen authored and zmodem committed Jul 27, 2020
1 parent 22324a6 commit 00ed535
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 31 deletions.
55 changes: 29 additions & 26 deletions llvm/lib/Analysis/BasicAliasAnalysis.cpp
View file
Expand Up @@ -1648,8 +1648,32 @@ AliasResult BasicAAResult::aliasPHI(const PHINode *PN, LocationSize PNSize,
}

SmallVector<Value *, 4> V1Srcs;
// For a recursive phi, that recurses through a contant gep, we can perform
// aliasing calculations using the other phi operands with an unknown size to
// specify that an unknown number of elements after the initial value are
// potentially accessed.
bool isRecursive = false;
if (PV) {
auto CheckForRecPhi = [&](Value *PV) {
if (!EnableRecPhiAnalysis)
return false;
if (GEPOperator *PVGEP = dyn_cast<GEPOperator>(PV)) {
// Check whether the incoming value is a GEP that advances the pointer
// result of this PHI node (e.g. in a loop). If this is the case, we
// would recurse and always get a MayAlias. Handle this case specially
// below. We need to ensure that the phi is inbounds and has a constant
// positive operand so that we can check for alias with the initial value
// and an unknown but positive size.
if (PVGEP->getPointerOperand() == PN && PVGEP->isInBounds() &&
PVGEP->getNumIndices() == 1 && isa<ConstantInt>(PVGEP->idx_begin()) &&
!cast<ConstantInt>(PVGEP->idx_begin())->isNegative()) {
isRecursive = true;
return true;
}
}
return false;
};

if (PV) {
// If we have PhiValues then use it to get the underlying phi values.
const PhiValues::ValueSet &PhiValueSet = PV->getValuesForPhi(PN);
// If we have more phi values than the search depth then return MayAlias
Expand All @@ -1660,19 +1684,8 @@ AliasResult BasicAAResult::aliasPHI(const PHINode *PN, LocationSize PNSize,
return MayAlias;
// Add the values to V1Srcs
for (Value *PV1 : PhiValueSet) {
if (EnableRecPhiAnalysis) {
if (GEPOperator *PV1GEP = dyn_cast<GEPOperator>(PV1)) {
// Check whether the incoming value is a GEP that advances the pointer
// result of this PHI node (e.g. in a loop). If this is the case, we
// would recurse and always get a MayAlias. Handle this case specially
// below.
if (PV1GEP->getPointerOperand() == PN && PV1GEP->getNumIndices() == 1 &&
isa<ConstantInt>(PV1GEP->idx_begin())) {
isRecursive = true;
continue;
}
}
}
if (CheckForRecPhi(PV1))
continue;
V1Srcs.push_back(PV1);
}
} else {
Expand All @@ -1687,18 +1700,8 @@ AliasResult BasicAAResult::aliasPHI(const PHINode *PN, LocationSize PNSize,
// and 'n' are the number of PHI sources.
return MayAlias;

if (EnableRecPhiAnalysis)
if (GEPOperator *PV1GEP = dyn_cast<GEPOperator>(PV1)) {
// Check whether the incoming value is a GEP that advances the pointer
// result of this PHI node (e.g. in a loop). If this is the case, we
// would recurse and always get a MayAlias. Handle this case specially
// below.
if (PV1GEP->getPointerOperand() == PN && PV1GEP->getNumIndices() == 1 &&
isa<ConstantInt>(PV1GEP->idx_begin())) {
isRecursive = true;
continue;
}
}
if (CheckForRecPhi(PV1))
continue;

if (UniqueSrc.insert(PV1).second)
V1Srcs.push_back(PV1);
Expand Down
10 changes: 5 additions & 5 deletions llvm/test/Analysis/BasicAA/recphi.ll
View file
Expand Up @@ -92,8 +92,8 @@ if.end: ; preds = %f.exit
; CHECK: NoAlias: i32* %arrayidx1, i8* %0
; CHECK: NoAlias: i32* %arrayidx, i32* %arrayidx1
; CHECK: MayAlias: [10 x i32]* %tab, i32* %p.addr.05.i
; CHECK: NoAlias: i32* %p.addr.05.i, i8* %0
; CHECK: NoAlias: i32* %arrayidx, i32* %p.addr.05.i
; CHECK: MayAlias: i32* %p.addr.05.i, i8* %0
; CHECK: MayAlias: i32* %arrayidx, i32* %p.addr.05.i
; CHECK: MayAlias: i32* %arrayidx1, i32* %p.addr.05.i
; CHECK: MayAlias: [10 x i32]* %tab, i32* %incdec.ptr.i
; CHECK: MayAlias: i32* %incdec.ptr.i, i8* %0
Expand Down Expand Up @@ -141,17 +141,17 @@ if.end: ; preds = %f.exit
; CHECK: NoAlias: [3 x i16]* %int_arr.10, i16** %argv.6.par
; CHECK: NoAlias: i16* %_tmp1, i16** %argv.6.par
; CHECK: PartialAlias: [3 x i16]* %int_arr.10, i16* %_tmp1
; CHECK: NoAlias: i16* %ls1.9.0, i16** %argv.6.par
; CHECK: MayAlias: i16* %ls1.9.0, i16** %argv.6.par
; CHECK: MayAlias: [3 x i16]* %int_arr.10, i16* %ls1.9.0
; CHECK: MayAlias: i16* %_tmp1, i16* %ls1.9.0
; CHECK: NoAlias: i16* %_tmp7, i16** %argv.6.par
; CHECK: MayAlias: i16* %_tmp7, i16** %argv.6.par
; CHECK: MayAlias: [3 x i16]* %int_arr.10, i16* %_tmp7
; CHECK: MayAlias: i16* %_tmp1, i16* %_tmp7
; CHECK: NoAlias: i16* %_tmp7, i16* %ls1.9.0
; CHECK: NoAlias: i16* %_tmp11, i16** %argv.6.par
; CHECK: PartialAlias: [3 x i16]* %int_arr.10, i16* %_tmp11
; CHECK: NoAlias: i16* %_tmp1, i16* %_tmp11
; CHECK: NoAlias: i16* %_tmp11, i16* %ls1.9.0
; CHECK: MayAlias: i16* %_tmp11, i16* %ls1.9.0
; CHECK: MayAlias: i16* %_tmp11, i16* %_tmp7
; CHECK: Both ModRef: Ptr: i16** %argv.6.par <-> %_tmp16 = call i16 @call(i32 %_tmp13)
; CHECK: NoModRef: Ptr: [3 x i16]* %int_arr.10 <-> %_tmp16 = call i16 @call(i32 %_tmp13)
Expand Down

0 comments on commit 00ed535

Please sign in to comment.