[llvm] r309651 - [StackColoring] Update AliasAnalysis information in stack coloring pass
Hal Finkel via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 1 10:46:07 PDT 2017
On 07/31/2017 10:32 PM, Hiroshi Inoue via llvm-commits wrote:
> Author: inouehrs
> Date: Mon Jul 31 20:32:15 2017
> New Revision: 309651
>
> URL: http://llvm.org/viewvc/llvm-project?rev=309651&view=rev
> Log:
> [StackColoring] Update AliasAnalysis information in stack coloring pass
>
> Stack coloring pass need to maintain AliasAnalysis information when merging stack slots of different types.
> Actually, there is a FIXME comment in StackColoring.cpp
>
> // FIXME: In order to enable the use of TBAA when using AA in CodeGen,
> // we'll also need to update the TBAA nodes in MMOs with values
> // derived from the merged allocas.
>
> But, TBAA has been already enabled in CodeGen without fixing this pass.
> The incorrect TBAA metadata results in recent failures in bootstrap test on ppc64le (PR33928) by allowing unsafe instruction scheduling.
> Although we observed the problem on ppc64le, this is a platform neutral issue.
>
> This patch makes the stack coloring pass maintains AliasAnalysis information when merging multiple stack slots.
>
>
> Modified:
> llvm/trunk/include/llvm/Analysis/ValueTracking.h
> llvm/trunk/include/llvm/CodeGen/MachineFunction.h
> llvm/trunk/include/llvm/CodeGen/MachineInstr.h
> llvm/trunk/lib/Analysis/ValueTracking.cpp
> llvm/trunk/lib/CodeGen/MachineFunction.cpp
> llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp
> llvm/trunk/lib/CodeGen/StackColoring.cpp
>
> Modified: llvm/trunk/include/llvm/Analysis/ValueTracking.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/ValueTracking.h?rev=309651&r1=309650&r2=309651&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/ValueTracking.h (original)
> +++ llvm/trunk/include/llvm/Analysis/ValueTracking.h Mon Jul 31 20:32:15 2017
> @@ -312,6 +312,12 @@ template <typename T> class ArrayRef;
> const DataLayout &DL, LoopInfo *LI = nullptr,
> unsigned MaxLookup = 6);
>
> + /// This is a wrapper around GetUnderlyingObjects and adds support for basic
> + /// ptrtoint+arithmetic+inttoptr sequences.
> + void getUnderlyingObjectsForCodeGen(const Value *V,
> + SmallVectorImpl<Value *> &Objects,
> + const DataLayout &DL);
> +
> /// Return true if the only users of this pointer are lifetime markers.
> bool onlyUsedByLifetimeMarkers(const Value *V);
>
>
> Modified: llvm/trunk/include/llvm/CodeGen/MachineFunction.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineFunction.h?rev=309651&r1=309650&r2=309651&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/MachineFunction.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineFunction.h Mon Jul 31 20:32:15 2017
> @@ -661,6 +661,12 @@ public:
> MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
> int64_t Offset, uint64_t Size);
>
> + /// Allocate a new MachineMemOperand by copying an existing one,
> + /// replacing only AliasAnalysis information. MachineMemOperands are owned
> + /// by the MachineFunction and need not be explicitly deallocated.
> + MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
> + const AAMDNodes &AAInfo);
> +
> using OperandCapacity = ArrayRecycler<MachineOperand>::Capacity;
>
> /// Allocate an array of MachineOperands. This is only intended for use by
>
> Modified: llvm/trunk/include/llvm/CodeGen/MachineInstr.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineInstr.h?rev=309651&r1=309650&r2=309651&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/MachineInstr.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineInstr.h Mon Jul 31 20:32:15 2017
> @@ -379,6 +379,9 @@ public:
> return NumMemRefs == 1;
> }
>
> + /// Return the number of memory operands.
> + unsigned getNumMemOperands() const { return NumMemRefs; }
> +
> /// API for querying MachineInstr properties. They are the same as MCInstrDesc
> /// queries but they are bundle aware.
>
>
> Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=309651&r1=309650&r2=309651&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
> +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Mon Jul 31 20:32:15 2017
> @@ -3277,6 +3277,70 @@ void llvm::GetUnderlyingObjects(Value *V
> } while (!Worklist.empty());
> }
>
> +/// This is the function that does the work of looking through basic
> +/// ptrtoint+arithmetic+inttoptr sequences.
> +static const Value *getUnderlyingObjectFromInt(const Value *V) {
> + do {
> + if (const Operator *U = dyn_cast<Operator>(V)) {
> + // If we find a ptrtoint, we can transfer control back to the
> + // regular getUnderlyingObjectFromInt.
> + if (U->getOpcode() == Instruction::PtrToInt)
> + return U->getOperand(0);
> + // If we find an add of a constant, a multiplied value, or a phi, it's
> + // likely that the other operand will lead us to the base
> + // object. We don't have to worry about the case where the
> + // object address is somehow being computed by the multiply,
> + // because our callers only care when the result is an
> + // identifiable object.
> + if (U->getOpcode() != Instruction::Add ||
> + (!isa<ConstantInt>(U->getOperand(1)) &&
> + Operator::getOpcode(U->getOperand(1)) != Instruction::Mul &&
> + !isa<PHINode>(U->getOperand(1))))
> + return V;
> + V = U->getOperand(0);
> + } else {
> + return V;
> + }
> + assert(V->getType()->isIntegerTy() && "Unexpected operand type!");
> + } while (true);
> +}
> +
> +/// This is a wrapper around GetUnderlyingObjects and adds support for basic
> +/// ptrtoint+arithmetic+inttoptr sequences.
> +void llvm::getUnderlyingObjectsForCodeGen(const Value *V,
> + SmallVectorImpl<Value *> &Objects,
> + const DataLayout &DL) {
> + SmallPtrSet<const Value *, 16> Visited;
> + SmallVector<const Value *, 4> Working(1, V);
> + do {
> + V = Working.pop_back_val();
> +
> + SmallVector<Value *, 4> Objs;
> + GetUnderlyingObjects(const_cast<Value *>(V), Objs, DL);
> +
> + for (Value *V : Objs) {
> + // If GetUnderlyingObjects fails to find an identifiable object,
> + // getUnderlyingObjectsForCodeGen also fails for safety.
> + if (!isIdentifiedObject(V)) {
> + Objects.clear();
> + return;
> + }
I don't think this check does that you want because, if V is a inttoptr,
this will always be false (i.e. the check for inttoptr will be dead).
> +
> + if (!Visited.insert(V).second)
> + continue;
> + if (Operator::getOpcode(V) == Instruction::IntToPtr) {
> + const Value *O =
> + getUnderlyingObjectFromInt(cast<User>(V)->getOperand(0));
> + if (O->getType()->isPointerTy()) {
> + Working.push_back(O);
> + continue;
> + }
> + }
To preserve functionality, I believe you need the check here instead.
-Hal
> + Objects.push_back(const_cast<Value *>(V));
> + }
> + } while (!Working.empty());
> +}
> +
> /// Return true if the only users of this pointer are lifetime markers.
> bool llvm::onlyUsedByLifetimeMarkers(const Value *V) {
> for (const User *U : V->users()) {
>
> Modified: llvm/trunk/lib/CodeGen/MachineFunction.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineFunction.cpp?rev=309651&r1=309650&r2=309651&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineFunction.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineFunction.cpp Mon Jul 31 20:32:15 2017
> @@ -330,6 +330,20 @@ MachineFunction::getMachineMemOperand(co
> MMO->getOrdering(), MMO->getFailureOrdering());
> }
>
> +MachineMemOperand *
> +MachineFunction::getMachineMemOperand(const MachineMemOperand *MMO,
> + const AAMDNodes &AAInfo) {
> + MachinePointerInfo MPI = MMO->getValue() ?
> + MachinePointerInfo(MMO->getValue(), MMO->getOffset()) :
> + MachinePointerInfo(MMO->getPseudoValue(), MMO->getOffset());
> +
> + return new (Allocator)
> + MachineMemOperand(MPI, MMO->getFlags(), MMO->getSize(),
> + MMO->getBaseAlignment(), AAInfo,
> + MMO->getRanges(), MMO->getSyncScopeID(),
> + MMO->getOrdering(), MMO->getFailureOrdering());
> +}
> +
> MachineInstr::mmo_iterator
> MachineFunction::allocateMemRefsArray(unsigned long Num) {
> return Allocator.Allocate<MachineMemOperand *>(Num);
>
> Modified: llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp?rev=309651&r1=309650&r2=309651&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp (original)
> +++ llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp Mon Jul 31 20:32:15 2017
> @@ -121,63 +121,6 @@ ScheduleDAGInstrs::ScheduleDAGInstrs(Mac
> SchedModel.init(ST.getSchedModel(), &ST, TII);
> }
>
> -/// This is the function that does the work of looking through basic
> -/// ptrtoint+arithmetic+inttoptr sequences.
> -static const Value *getUnderlyingObjectFromInt(const Value *V) {
> - do {
> - if (const Operator *U = dyn_cast<Operator>(V)) {
> - // If we find a ptrtoint, we can transfer control back to the
> - // regular getUnderlyingObjectFromInt.
> - if (U->getOpcode() == Instruction::PtrToInt)
> - return U->getOperand(0);
> - // If we find an add of a constant, a multiplied value, or a phi, it's
> - // likely that the other operand will lead us to the base
> - // object. We don't have to worry about the case where the
> - // object address is somehow being computed by the multiply,
> - // because our callers only care when the result is an
> - // identifiable object.
> - if (U->getOpcode() != Instruction::Add ||
> - (!isa<ConstantInt>(U->getOperand(1)) &&
> - Operator::getOpcode(U->getOperand(1)) != Instruction::Mul &&
> - !isa<PHINode>(U->getOperand(1))))
> - return V;
> - V = U->getOperand(0);
> - } else {
> - return V;
> - }
> - assert(V->getType()->isIntegerTy() && "Unexpected operand type!");
> - } while (true);
> -}
> -
> -/// This is a wrapper around GetUnderlyingObjects and adds support for basic
> -/// ptrtoint+arithmetic+inttoptr sequences.
> -static void getUnderlyingObjects(const Value *V,
> - SmallVectorImpl<Value *> &Objects,
> - const DataLayout &DL) {
> - SmallPtrSet<const Value *, 16> Visited;
> - SmallVector<const Value *, 4> Working(1, V);
> - do {
> - V = Working.pop_back_val();
> -
> - SmallVector<Value *, 4> Objs;
> - GetUnderlyingObjects(const_cast<Value *>(V), Objs, DL);
> -
> - for (Value *V : Objs) {
> - if (!Visited.insert(V).second)
> - continue;
> - if (Operator::getOpcode(V) == Instruction::IntToPtr) {
> - const Value *O =
> - getUnderlyingObjectFromInt(cast<User>(V)->getOperand(0));
> - if (O->getType()->isPointerTy()) {
> - Working.push_back(O);
> - continue;
> - }
> - }
> - Objects.push_back(const_cast<Value *>(V));
> - }
> - } while (!Working.empty());
> -}
> -
> /// If this machine instr has memory reference information and it can be tracked
> /// to a normal reference to a known object, return the Value for that object.
> static void getUnderlyingObjectsForInstr(const MachineInstr *MI,
> @@ -208,12 +151,10 @@ static void getUnderlyingObjectsForInstr
> Objects.push_back(UnderlyingObjectsVector::value_type(PSV, MayAlias));
> } else if (const Value *V = MMO->getValue()) {
> SmallVector<Value *, 4> Objs;
> - getUnderlyingObjects(V, Objs, DL);
> + getUnderlyingObjectsForCodeGen(V, Objs, DL);
>
> for (Value *V : Objs) {
> - if (!isIdentifiedObject(V))
> - return false;
> -
> + assert(isIdentifiedObject(V));
> Objects.push_back(UnderlyingObjectsVector::value_type(V, true));
> }
> } else
>
> Modified: llvm/trunk/lib/CodeGen/StackColoring.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/StackColoring.cpp?rev=309651&r1=309650&r2=309651&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/StackColoring.cpp (original)
> +++ llvm/trunk/lib/CodeGen/StackColoring.cpp Mon Jul 31 20:32:15 2017
> @@ -37,6 +37,7 @@
> #include "llvm/CodeGen/MachineRegisterInfo.h"
> #include "llvm/CodeGen/Passes.h"
> #include "llvm/CodeGen/PseudoSourceValue.h"
> +#include "llvm/CodeGen/SelectionDAGNodes.h"
> #include "llvm/CodeGen/SlotIndexes.h"
> #include "llvm/CodeGen/StackProtector.h"
> #include "llvm/CodeGen/WinEHFuncInfo.h"
> @@ -889,6 +890,10 @@ void StackColoring::remapInstructions(De
>
> // Keep a list of *allocas* which need to be remapped.
> DenseMap<const AllocaInst*, const AllocaInst*> Allocas;
> +
> + // Keep a list of allocas which has been affected by the remap.
> + SmallPtrSet<const AllocaInst*, 32> MergedAllocas;
> +
> for (const std::pair<int, int> &SI : SlotRemap) {
> const AllocaInst *From = MFI->getObjectAllocation(SI.first);
> const AllocaInst *To = MFI->getObjectAllocation(SI.second);
> @@ -908,6 +913,10 @@ void StackColoring::remapInstructions(De
> Inst = Cast;
> }
>
> + // We keep both slots to maintain AliasAnalysis metadata later.
> + MergedAllocas.insert(From);
> + MergedAllocas.insert(To);
> +
> // Allow the stack protector to adjust its value map to account for the
> // upcoming replacement.
> SP->adjustForColoring(From, To);
> @@ -939,13 +948,6 @@ void StackColoring::remapInstructions(De
>
> // Update the MachineMemOperand to use the new alloca.
> for (MachineMemOperand *MMO : I.memoperands()) {
> - // FIXME: In order to enable the use of TBAA when using AA in CodeGen,
> - // we'll also need to update the TBAA nodes in MMOs with values
> - // derived from the merged allocas. When doing this, we'll need to use
> - // the same variant of GetUnderlyingObjects that is used by the
> - // instruction scheduler (that can look through ptrtoint/inttoptr
> - // pairs).
> -
> // We've replaced IR-level uses of the remapped allocas, so we only
> // need to replace direct uses here.
> const AllocaInst *AI = dyn_cast_or_null<AllocaInst>(MMO->getValue());
> @@ -997,6 +999,48 @@ void StackColoring::remapInstructions(De
> MO.setIndex(ToSlot);
> FixedInstr++;
> }
> +
> + // We adjust AliasAnalysis information for merged stack slots.
> + MachineSDNode::mmo_iterator MemOps =
> + MF->allocateMemRefsArray(I.getNumMemOperands());
> + unsigned MemOpIdx = 0;
> + bool ReplaceMemOps = false;
> + for (MachineMemOperand *MMO : I.memoperands()) {
> + // If this memory location can be a slot remapped here,
> + // we remove AA information.
> + bool MayHaveConflictingAAMD = false;
> + if (MMO->getAAInfo()) {
> + if (const Value *MMOV = MMO->getValue()) {
> + SmallVector<Value *, 4> Objs;
> + getUnderlyingObjectsForCodeGen(MMOV, Objs, MF->getDataLayout());
> +
> + if (Objs.empty())
> + MayHaveConflictingAAMD = true;
> + else
> + for (Value *V : Objs) {
> + // If this memory location comes from a known stack slot
> + // that is not remapped, we continue checking.
> + // Otherwise, we need to invalidate AA infomation.
> + const AllocaInst *AI = dyn_cast_or_null<AllocaInst>(V);
> + if (AI && MergedAllocas.count(AI)) {
> + MayHaveConflictingAAMD = true;
> + break;
> + }
> + }
> + }
> + }
> + if (MayHaveConflictingAAMD) {
> + MemOps[MemOpIdx++] = MF->getMachineMemOperand(MMO, AAMDNodes());
> + ReplaceMemOps = true;
> + }
> + else
> + MemOps[MemOpIdx++] = MMO;
> + }
> +
> + // If any memory operand is updated, set memory references of
> + // this instruction.
> + if (ReplaceMemOps)
> + I.setMemRefs(std::make_pair(MemOps, I.getNumMemOperands()));
> }
>
> // Update the location of C++ catch objects for the MSVC personality routine.
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list