[llvm] r309651 - [StackColoring] Update AliasAnalysis information in stack coloring pass
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 2 04:34:01 PDT 2017
Hi Hiroshi, Matthias and Hal,
(CC my colleague Mattias)
I think I've found a problem with this patch.
Found it on my out-of-tree target, but I'll try to describe what's
happening.
The problem is during scheduling where we fail to identify a dependency
between a store and a following load. The store _might_ store at the
same address as the load loads from, but it doesn't always do it. It
depends on earlier code flow.
The store looks like:
mv_a32_r16_rmod1 %a0_32, %r1; mem:ST2[%_tmp9] ST2[@g_290]
I.e. it has two memops describing the two different memory locations it
might store to (is this weird?). They memory locations are completely
unrelated and describe the two locations the pointer %r1 can point to.
Before this patch, when ScheduleDAGInstrs::buildSchedGraph called
UnderlyingObjectsVector Objs;
getUnderlyingObjectsForInstr(&MI, MFI, Objs, MF.getDataLayout());
it got an empty Objs vector back since getUnderlyingObjectsForInstr
found one non-identified object (%_tmp9) and then cleared the whole vector.
With this patch, I instead get a non-empty Objs, containing only @g_290
and because of that we don't add a chain dependency towards the load
following the store, and we get wrong code as a result.
I know there is code in the method getUnderlyingObjectsForCodeGen that
looks for non-identified objects and if found it clears the Objects
arrays, but the way it's implemented it doesn't help!
In my current case where there are two memops and the first one is
non-identified, we still continue with the second memop afterwards and
add that underlying object to the vector, and leave that to the
schedgraph builder where we don't see any traces of any non-identified
object, and we build a graph with missing dependencies
I suppose this difference is not intended?
Regards,
Mikael
On 08/01/2017 05:32 AM, 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;
> + }
> +
> + 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());
> +}
> +
> /// 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
>
More information about the llvm-commits
mailing list