From 311fafd2c90aed5b3fed9566503eebe629f1e979 Mon Sep 17 00:00:00 2001 From: David Green Date: Thu, 16 Jul 2020 15:42:05 +0100 Subject: [PATCH] [BasicAA] Fix -basicaa-recphi for geps with negative offsets 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 --- llvm/lib/Analysis/BasicAliasAnalysis.cpp | 55 +++++++++++++----------- llvm/test/Analysis/BasicAA/recphi.ll | 10 ++--- 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp index 74664098ce1d..33f122728d2a 100644 --- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp +++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp @@ -1648,8 +1648,32 @@ AliasResult BasicAAResult::aliasPHI(const PHINode *PN, LocationSize PNSize, } SmallVector 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(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(PVGEP->idx_begin()) && + !cast(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 @@ -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(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(PV1GEP->idx_begin())) { - isRecursive = true; - continue; - } - } - } + if (CheckForRecPhi(PV1)) + continue; V1Srcs.push_back(PV1); } } else { @@ -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(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(PV1GEP->idx_begin())) { - isRecursive = true; - continue; - } - } + if (CheckForRecPhi(PV1)) + continue; if (UniqueSrc.insert(PV1).second) V1Srcs.push_back(PV1); diff --git a/llvm/test/Analysis/BasicAA/recphi.ll b/llvm/test/Analysis/BasicAA/recphi.ll index 3c8397cc0fec..dfc88937bf69 100644 --- a/llvm/test/Analysis/BasicAA/recphi.ll +++ b/llvm/test/Analysis/BasicAA/recphi.ll @@ -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 @@ -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)