[llvm] r251451 - [AliasSetTracker] Use mod/ref information for UnknownInstr

Wed Oct 28 15:20:27 PDT 2015

Thanks Hal.

—Juergen

> On Oct 28, 2015, at 3:18 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> ----- Original Message -----
>> From: "Juergen Ributzka" <juergen at apple.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: llvm-commits at lists.llvm.org
>> Sent: Wednesday, October 28, 2015 4:56:59 PM
>> Subject: Re: [llvm] r251451 - [AliasSetTracker] Use mod/ref information for UnknownInstr
>> 
>> Hi Hal,
>> 
>> 
>> looks like this change broke the stage2 compiler:
>> http://lab.llvm.org:8080/green/job/clang-stage2-configure-Rlto/6989/
>> 
>> 
>> Could you please revert the patch?
>> 
> 
> Reverted in r251562.
> 
> -Hal
> 
>> 
>> Thanks
>> 
>> 
>> Cheers,
>> Juergen
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> On Oct 27, 2015, at 1:37 PM, Hal Finkel via llvm-commits <
>> llvm-commits at lists.llvm.org > wrote:
>> 
>> 
>> Author: hfinkel
>> Date: Tue Oct 27 15:37:04 2015
>> New Revision: 251451
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=251451&view=rev
>> Log:
>> [AliasSetTracker] Use mod/ref information for UnknownInstr
>> 
>> AliasSetTracker does not need to convert the access mode to
>> ModRefAccess if the
>> new visited UnknownInst has only 'REF' modrefinfo to existing
>> pointers in the
>> sets.
>> 
>> Patch by Andrew Zhogin!
>> 
>> Added:
>> llvm/trunk/test/Transforms/LICM/licm_call_referenced_ptr.ll
>> Modified:
>> llvm/trunk/include/llvm/Analysis/AliasSetTracker.h
>> llvm/trunk/lib/Analysis/AliasSetTracker.cpp
>> 
>> Modified: llvm/trunk/include/llvm/Analysis/AliasSetTracker.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/AliasSetTracker.h?rev=251451&r1=251450&r2=251451&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Analysis/AliasSetTracker.h (original)
>> +++ llvm/trunk/include/llvm/Analysis/AliasSetTracker.h Tue Oct 27
>> 15:37:04 2015
>> @@ -124,12 +124,7 @@ class AliasSet : public ilist_node<Alias
>> /// memory (and not any particular access), whether it modifies or
>> references
>> /// the memory, or whether it does both. The lattice goes from
>> "NoAccess" to
>> /// either RefAccess or ModAccess, then to ModRefAccess as necessary.
>> - enum AccessLattice {
>> - NoAccess = 0,
>> - RefAccess = 1,
>> - ModAccess = 2,
>> - ModRefAccess = RefAccess | ModAccess
>> - };
>> + typedef ModRefInfo AccessLattice;
>> unsigned Access : 2;
>> 
>> /// The kind of alias relationship between pointers of the set.
>> @@ -160,8 +155,8 @@ class AliasSet : public ilist_node<Alias
>> 
>> public:
>> /// Accessors...
>> - bool isRef() const { return Access & RefAccess; }
>> - bool isMod() const { return Access & ModAccess; }
>> + bool isRef() const { return Access & MRI_Ref; }
>> + bool isMod() const { return Access & MRI_Mod; }
>> bool isMustAlias() const { return Alias == SetMustAlias; }
>> bool isMayAlias() const { return Alias == SetMayAlias; }
>> 
>> @@ -226,7 +221,7 @@ private:
>> friend struct ilist_sentinel_traits<AliasSet>;
>> AliasSet()
>> : PtrList(nullptr), PtrListEnd(&PtrList), Forward(nullptr),
>> RefCount(0),
>> - Access(NoAccess), Alias(SetMustAlias), Volatile(false) {
>> + Access(MRI_NoModRef), Alias(SetMustAlias), Volatile(false) {
>> }
>> 
>> AliasSet(const AliasSet &AS) = delete;
>> @@ -254,9 +249,9 @@ private:
>> void removeFromTracker(AliasSetTracker &AST);
>> 
>> void addPointer(AliasSetTracker &AST, PointerRec &Entry, uint64_t
>> Size,
>> - const AAMDNodes &AAInfo,
>> + const AAMDNodes &AAInfo, ModRefInfo MR,
>> bool KnownMustAlias = false);
>> - void addUnknownInst(Instruction *I, AliasAnalysis &AA);
>> + void addUnknownInst(Instruction *I, AliasAnalysis &AA, ModRefInfo
>> MR);
>> void removeUnknownInst(AliasSetTracker &AST, Instruction *I) {
>> bool WasEmpty = UnknownInsts.empty();
>> for (size_t i = 0, e = UnknownInsts.size(); i != e; ++i)
>> @@ -273,10 +268,18 @@ private:
>> public:
>> /// aliasesPointer - Return true if the specified pointer "may" (or
>> must)
>> /// alias one of the members in the set.
>> - ///
>> + /// MRcommon - if Ptr is aliased by existing UnknownInsts,
>> + /// then not-null MRcommon will be set to the worst ModRefInfo met
>> + ///
>> bool aliasesPointer(const Value *Ptr, uint64_t Size, const AAMDNodes
>> &AAInfo,
>> - AliasAnalysis &AA) const;
>> - bool aliasesUnknownInst(const Instruction *Inst, AliasAnalysis &AA)
>> const;
>> + AliasAnalysis &AA, ModRefInfo* MRcommon = nullptr) const;
>> + /// aliasesUnknownInst - Return true if the specified UnknownInst
>> + /// has not-null ModRefInfo (not MRI_NoModRef) with some
>> + /// pointer or UnknownInst already existing in AliasSet
>> + /// MRcommon - not-null MRcommon will be set to the worst
>> ModRefInfo met
>> + ///
>> + bool aliasesUnknownInst(const Instruction *Inst, AliasAnalysis &AA,
>> + ModRefInfo* MRcommon = nullptr) const;
>> };
>> 
>> inline raw_ostream& operator<<(raw_ostream &OS, const AliasSet &AS) {
>> @@ -434,9 +437,11 @@ private:
>> return AS;
>> }
>> AliasSet *findAliasSetForPointer(const Value *Ptr, uint64_t Size,
>> - const AAMDNodes &AAInfo);
>> + const AAMDNodes &AAInfo,
>> + ModRefInfo* MRcommonPtr = nullptr);
>> 
>> - AliasSet *findAliasSetForUnknownInst(Instruction *Inst);
>> + AliasSet *findAliasSetForUnknownInst(Instruction *Inst,
>> + ModRefInfo* MRcommonPtr = nullptr);
>> };
>> 
>> inline raw_ostream& operator<<(raw_ostream &OS, const AliasSetTracker
>> &AST) {
>> 
>> Modified: llvm/trunk/lib/Analysis/AliasSetTracker.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/AliasSetTracker.cpp?rev=251451&r1=251450&r2=251451&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Analysis/AliasSetTracker.cpp (original)
>> +++ llvm/trunk/lib/Analysis/AliasSetTracker.cpp Tue Oct 27 15:37:04
>> 2015
>> @@ -95,7 +95,7 @@ void AliasSet::removeFromTracker(AliasSe
>> 
>> void AliasSet::addPointer(AliasSetTracker &AST, PointerRec &Entry,
>> uint64_t Size, const AAMDNodes &AAInfo,
>> - bool KnownMustAlias) {
>> + ModRefInfo MR, bool KnownMustAlias) {
>> assert(!Entry.hasAliasSet() && "Entry already in set!");
>> 
>> // Check to see if we have to downgrade to _may_ alias.
>> @@ -112,6 +112,9 @@ void AliasSet::addPointer(AliasSetTracke
>> assert(Result != NoAlias && "Cannot be part of must set!");
>> }
>> 
>> + // upgrading access if existing UnknownInst has ModRef with new
>> pointer
>> + Access |= MR;
>> +
>> Entry.setAliasSet(this);
>> Entry.updateSizeAndAAInfo(Size, AAInfo);
>> 
>> @@ -123,20 +126,34 @@ void AliasSet::addPointer(AliasSetTracke
>> addRef(); // Entry points to alias set.
>> }
>> 
>> -void AliasSet::addUnknownInst(Instruction *I, AliasAnalysis &AA) {
>> +void AliasSet::addUnknownInst(Instruction *I, AliasAnalysis &AA,
>> + ModRefInfo MR) {
>> if (UnknownInsts.empty())
>> addRef();
>> UnknownInsts.emplace_back(I);
>> 
>> if (!I->mayWriteToMemory()) {
>> Alias = SetMayAlias;
>> - Access |= RefAccess;
>> + Access |= MRI_Ref;
>> return;
>> }
>> 
>> - // FIXME: This should use mod/ref information to make this not suck
>> so bad
>> Alias = SetMayAlias;
>> - Access = ModRefAccess;
>> + Access |= MR;
>> +}
>> +
>> +namespace {
>> + /// returns true if there is no request of worst ModRefInfo
>> + /// (MRcommonPtr is null)
>> + /// or when achieved maximum ModRefInfo (MRI_ModRef).
>> + bool processMR(ModRefInfo MR, ModRefInfo* MRcommonPtr, ModRefInfo&
>> MRcommon) {
>> + MRcommon = ModRefInfo(MRcommon | MR);
>> + return !MRcommonPtr || (MRcommon == MRI_ModRef);
>> + }
>> + bool fillExitMR(ModRefInfo* MRcommonPtr, ModRefInfo& MRcommon) {
>> + if (MRcommonPtr) *MRcommonPtr = MRcommon;
>> + return MRcommon != MRI_NoModRef;
>> + }
>> }
>> 
>> /// aliasesPointer - Return true if the specified pointer "may" (or
>> must)
>> @@ -144,7 +161,8 @@ void AliasSet::addUnknownInst(Instructio
>> ///
>> bool AliasSet::aliasesPointer(const Value *Ptr, uint64_t Size,
>> const AAMDNodes &AAInfo,
>> - AliasAnalysis &AA) const {
>> + AliasAnalysis &AA,
>> + ModRefInfo* MRcommonPtr) const {
>> if (Alias == SetMustAlias) {
>> assert(UnknownInsts.empty() && "Illegal must alias set!");
>> 
>> @@ -164,35 +182,44 @@ bool AliasSet::aliasesPointer(const Valu
>> MemoryLocation(I.getPointer(), I.getSize(), I.getAAInfo())))
>> return true;
>> 
>> + // to gather worst ModRefInfo
>> + ModRefInfo MRcommon = MRI_NoModRef;
>> +
>> // Check the unknown instructions...
>> if (!UnknownInsts.empty()) {
>> for (unsigned i = 0, e = UnknownInsts.size(); i != e; ++i)
>> - if (AA.getModRefInfo(UnknownInsts[i],
>> - MemoryLocation(Ptr, Size, AAInfo)) != MRI_NoModRef)
>> - return true;
>> + if (processMR(AA.getModRefInfo(UnknownInsts[i],
>> + MemoryLocation(Ptr, Size, AAInfo)), MRcommonPtr, MRcommon))
>> + return fillExitMR(MRcommonPtr, MRcommon);
>> }
>> 
>> - return false;
>> + return fillExitMR(MRcommonPtr, MRcommon);
>> }
>> 
>> bool AliasSet::aliasesUnknownInst(const Instruction *Inst,
>> - AliasAnalysis &AA) const {
>> + AliasAnalysis &AA,
>> + ModRefInfo* MRcommonPtr) const {
>> if (!Inst->mayReadOrWriteMemory())
>> return false;
>> 
>> + // to gather worst ModRefInfo
>> + ModRefInfo MRcommon = MRI_NoModRef;
>> +
>> for (unsigned i = 0, e = UnknownInsts.size(); i != e; ++i) {
>> ImmutableCallSite C1(getUnknownInst(i)), C2(Inst);
>> - if (!C1 || !C2 || AA.getModRefInfo(C1, C2) != MRI_NoModRef ||
>> - AA.getModRefInfo(C2, C1) != MRI_NoModRef)
>> - return true;
>> + if (!C1 || !C2 ||
>> + processMR(AA.getModRefInfo(C1, C2), MRcommonPtr, MRcommon) ||
>> + processMR(AA.getModRefInfo(C2, C1), MRcommonPtr, MRcommon))
>> + return fillExitMR(MRcommonPtr, MRcommon);
>> }
>> 
>> - for (iterator I = begin(), E = end(); I != E; ++I)
>> - if (AA.getModRefInfo(Inst, MemoryLocation(I.getPointer(),
>> I.getSize(),
>> - I.getAAInfo())) != MRI_NoModRef)
>> - return true;
>> -
>> - return false;
>> + for (iterator I = begin(), E = end(); I != E; ++I) {
>> + ModRefInfo MR = AA.getModRefInfo(
>> + Inst, MemoryLocation(I.getPointer(), I.getSize(), I.getAAInfo()));
>> + if (processMR(MR, MRcommonPtr, MRcommon))
>> + return fillExitMR(MRcommonPtr, MRcommon);
>> + }
>> + return fillExitMR(MRcommonPtr, MRcommon);
>> }
>> 
>> void AliasSetTracker::clear() {
>> @@ -214,11 +241,15 @@ void AliasSetTracker::clear() {
>> ///
>> AliasSet *AliasSetTracker::findAliasSetForPointer(const Value *Ptr,
>> uint64_t Size,
>> - const AAMDNodes &AAInfo) {
>> + const AAMDNodes &AAInfo,
>> + ModRefInfo* MRcommonPtr) {
>> AliasSet *FoundSet = nullptr;
>> for (iterator I = begin(), E = end(); I != E;) {
>> iterator Cur = I++;
>> - if (Cur->Forward || !Cur->aliasesPointer(Ptr, Size, AAInfo, AA))
>> continue;
>> + ModRefInfo MR = MRI_NoModRef;
>> + if (Cur->Forward || !Cur->aliasesPointer(Ptr, Size, AAInfo, AA,
>> &MR))
>> + continue;
>> + *MRcommonPtr = ModRefInfo(*MRcommonPtr | MR);
>> 
>> if (!FoundSet) { // If this is the first alias set ptr can go into.
>> FoundSet = &*Cur; // Remember it.
>> @@ -248,12 +279,15 @@ bool AliasSetTracker::containsUnknown(co
>> return false;
>> }
>> 
>> -AliasSet *AliasSetTracker::findAliasSetForUnknownInst(Instruction
>> *Inst) {
>> +AliasSet *AliasSetTracker::findAliasSetForUnknownInst(Instruction
>> *Inst,
>> + ModRefInfo* MRcommonPtr) {
>> AliasSet *FoundSet = nullptr;
>> for (iterator I = begin(), E = end(); I != E;) {
>> iterator Cur = I++;
>> - if (Cur->Forward || !Cur->aliasesUnknownInst(Inst, AA))
>> + ModRefInfo MR = MRI_NoModRef;
>> + if (Cur->Forward || !Cur->aliasesUnknownInst(Inst, AA, &MR))
>> continue;
>> + *MRcommonPtr = ModRefInfo(*MRcommonPtr | MR);
>> if (!FoundSet) // If this is the first alias set ptr can go into.
>> FoundSet = &*Cur; // Remember it.
>> else if (!Cur->Forward) // Otherwise, we must merge the sets.
>> @@ -279,22 +313,23 @@ AliasSet &AliasSetTracker::getAliasSetFo
>> return *Entry.getAliasSet(*this)->getForwardedTarget(*this);
>> }
>> 
>> - if (AliasSet *AS = findAliasSetForPointer(Pointer, Size, AAInfo)) {
>> + ModRefInfo MR = MRI_NoModRef;
>> + if (AliasSet *AS = findAliasSetForPointer(Pointer, Size, AAInfo,
>> &MR)) {
>> // Add it to the alias set it aliases.
>> - AS->addPointer(*this, Entry, Size, AAInfo);
>> + AS->addPointer(*this, Entry, Size, AAInfo, MR);
>> return *AS;
>> }
>> 
>> if (New) *New = true;
>> // Otherwise create a new alias set to hold the loaded pointer.
>> AliasSets.push_back(new AliasSet());
>> - AliasSets.back().addPointer(*this, Entry, Size, AAInfo);
>> + AliasSets.back().addPointer(*this, Entry, Size, AAInfo,
>> MRI_NoModRef);
>> return AliasSets.back();
>> }
>> 
>> bool AliasSetTracker::add(Value *Ptr, uint64_t Size, const AAMDNodes
>> &AAInfo) {
>> bool NewPtr;
>> - addPointer(Ptr, Size, AAInfo, AliasSet::NoAccess, NewPtr);
>> + addPointer(Ptr, Size, AAInfo, MRI_NoModRef, NewPtr);
>> return NewPtr;
>> }
>> 
>> @@ -305,7 +340,7 @@ bool AliasSetTracker::add(LoadInst *LI)
>> AAMDNodes AAInfo;
>> LI->getAAMetadata(AAInfo);
>> 
>> - AliasSet::AccessLattice Access = AliasSet::RefAccess;
>> + AliasSet::AccessLattice Access = MRI_Ref;
>> bool NewPtr;
>> const DataLayout &DL = LI->getModule()->getDataLayout();
>> AliasSet &AS = addPointer(LI->getOperand(0),
>> @@ -321,7 +356,7 @@ bool AliasSetTracker::add(StoreInst *SI)
>> AAMDNodes AAInfo;
>> SI->getAAMetadata(AAInfo);
>> 
>> - AliasSet::AccessLattice Access = AliasSet::ModAccess;
>> + AliasSet::AccessLattice Access = MRI_Mod;
>> bool NewPtr;
>> const DataLayout &DL = SI->getModule()->getDataLayout();
>> Value *Val = SI->getOperand(0);
>> @@ -338,7 +373,7 @@ bool AliasSetTracker::add(VAArgInst *VAA
>> 
>> bool NewPtr;
>> addPointer(VAAI->getOperand(0), MemoryLocation::UnknownSize, AAInfo,
>> - AliasSet::ModRefAccess, NewPtr);
>> + MRI_ModRef, NewPtr);
>> return NewPtr;
>> }
>> 
>> @@ -349,14 +384,15 @@ bool AliasSetTracker::addUnknown(Instruc
>> if (!Inst->mayReadOrWriteMemory())
>> return true; // doesn't alias anything
>> 
>> - AliasSet *AS = findAliasSetForUnknownInst(Inst);
>> + ModRefInfo MR = MRI_NoModRef;
>> + AliasSet *AS = findAliasSetForUnknownInst(Inst, &MR);
>> if (AS) {
>> - AS->addUnknownInst(Inst, AA);
>> + AS->addUnknownInst(Inst, AA, MR);
>> return false;
>> }
>> AliasSets.push_back(new AliasSet());
>> AS = &AliasSets.back();
>> - AS->addUnknownInst(Inst, AA);
>> + AS->addUnknownInst(Inst, AA, MR);
>> return true;
>> }
>> 
>> @@ -556,7 +592,7 @@ void AliasSetTracker::copyValue(Value *F
>> I = PointerMap.find_as(From);
>> AliasSet *AS = I->second->getAliasSet(*this);
>> AS->addPointer(*this, Entry, I->second->getSize(),
>> - I->second->getAAInfo(),
>> + I->second->getAAInfo(), MRI_NoModRef,
>> true);
>> }
>> 
>> @@ -570,10 +606,10 @@ void AliasSet::print(raw_ostream &OS) co
>> OS << " AliasSet[" << (const void*)this << ", " << RefCount << "] ";
>> OS << (Alias == SetMustAlias ? "must" : "may") << " alias, ";
>> switch (Access) {
>> - case NoAccess: OS << "No access "; break;
>> - case RefAccess: OS << "Ref "; break;
>> - case ModAccess: OS << "Mod "; break;
>> - case ModRefAccess: OS << "Mod/Ref "; break;
>> + case MRI_NoModRef: OS << "No access "; break;
>> + case MRI_Ref: OS << "Ref "; break;
>> + case MRI_Mod: OS << "Mod "; break;
>> + case MRI_ModRef: OS << "Mod/Ref "; break;
>> default: llvm_unreachable("Bad value for Access!");
>> }
>> if (isVolatile()) OS << "[volatile] ";
>> 
>> Added: llvm/trunk/test/Transforms/LICM/licm_call_referenced_ptr.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LICM/licm_call_referenced_ptr.ll?rev=251451&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/LICM/licm_call_referenced_ptr.ll
>> (added)
>> +++ llvm/trunk/test/Transforms/LICM/licm_call_referenced_ptr.ll Tue
>> Oct 27 15:37:04 2015
>> @@ -0,0 +1,40 @@
>> +; RUN: opt < %s -basicaa -licm -S | FileCheck %s
>> +
>> +declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8*
>> nocapture, i64, i32, i1) nounwind
>> +
>> +define i8 @"main"() {
>> +entry:
>> + %A = alloca [64 x i8]
>> + %B = alloca [4 x i8]
>> + %A0 = getelementptr [64 x i8], [64 x i8]* %A, i32 0, i32 0
>> + %B0 = getelementptr [4 x i8], [4 x i8]* %B, i32 0, i32 0
>> + %B1 = getelementptr [4 x i8], [4 x i8]* %B, i32 0, i32 1
>> + %B2 = getelementptr [4 x i8], [4 x i8]* %B, i32 0, i32 2
>> + %B3 = getelementptr [4 x i8], [4 x i8]* %B, i32 0, i32 3
>> + store i8 0, i8* %A0
>> + store i8 32, i8* %B0
>> + store i8 73, i8* %B1
>> + store i8 74, i8* %B2
>> + store i8 75, i8* %B3
>> + br label %loop_begin
>> +
>> +loop_begin:
>> +; CHECK: loop_begin:
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %A0, i8* %B0,
>> i64 4, i32 4, i1 false)
>> +
>> + %b_val = load i8, i8* %B0
>> +
>> + ; *B is invariant in loop and limit_val must be hoisted
>> + %limit_val_1 = mul i8 %b_val, 3
>> + %limit_val = add i8 %limit_val_1, 67
>> +
>> + call void @llvm.memcpy.p0i8.p0i8.i64(i8* %A0, i8* %B0, i64 4, i32
>> 4, i1 false)
>> +
>> + %exitcond = icmp ugt i8 164, %limit_val
>> + br i1 %exitcond, label %after_loop, label %loop_begin
>> +
>> +after_loop:
>> + %b_val_result = load i8, i8* %B0
>> + ret i8 %b_val_result
>> +}
>> +
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> 
>> 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151028/6c51dc31/attachment.html>