Skip to content

Commit

Permalink
[AAPointerInfo] refactor how offsets and Access objects are tracked
Browse files Browse the repository at this point in the history
AAPointerInfo now maintains a list of all Access objects that it owns, along
with the following maps:

- OffsetBins: OffsetAndSize -> { Access }
- InstTupleMap: RemoteI x LocalI -> Access

A RemoteI is any instruction that accesses memory. RemoteI is different from
LocalI if and only if LocalI is a call; then RemoteI is some instruction in the
callgraph starting from LocalI.

Motivation: When AAPointerInfo recomputes the offset for an instruction, it sets
the value to Unknown if the new offset is not the same as the old offset. The
instruction must now be moved from its current bin to the bin corresponding to
the new offset. This happens for example, when:

- A PHINode has operands that result in different offsets.
- The same remote inst is reachable from the same local inst via different paths
  in the callgraph:

```
               A (local inst)
               |
               B
              / \
             C1  C2
              \ /
               D (remote inst)

```
This fixes a bug where a store is incorrectly eliminated in a lit test.

Reviewed By: jdoerfert

Differential Revision: https://reviews.llvm.org/D136526
  • Loading branch information
ssahasra committed Nov 1, 2022
1 parent 412c4a8 commit b756096
Show file tree
Hide file tree
Showing 5 changed files with 320 additions and 145 deletions.
61 changes: 50 additions & 11 deletions llvm/include/llvm/Transforms/IPO/Attributor.h
Expand Up @@ -250,6 +250,22 @@ struct OffsetAndSize {
return OAS.Offset + OAS.Size > Offset && OAS.Offset < Offset + Size;
}

OffsetAndSize &operator&=(const OffsetAndSize &R) {
if (Offset == Unassigned)
Offset = R.Offset;
else if (R.Offset != Unassigned && R.Offset != Offset)
Offset = Unknown;

if (Size == Unassigned)
Size = R.Size;
else if (Size == Unknown || R.Size == Unknown)
Size = Unknown;
else if (R.Size != Unassigned)
Size = std::max(Size, R.Size);

return *this;
}

/// Constants used to represent special offsets or sizes.
/// - This assumes that Offset and Size are non-negative.
/// - The constants should not clash with DenseMapInfo, such as EmptyKey
Expand Down Expand Up @@ -4992,33 +5008,47 @@ struct AAPointerInfo : public AbstractAttribute {

/// An access description.
struct Access {
Access(Instruction *I, Optional<Value *> Content, AccessKind Kind, Type *Ty)
: LocalI(I), RemoteI(I), Content(Content), Kind(Kind), Ty(Ty) {
Access(Instruction *I, int64_t Offset, int64_t Size,
Optional<Value *> Content, AccessKind Kind, Type *Ty)
: LocalI(I), RemoteI(I), Content(Content), OAS(Offset, Size),
Kind(Kind), Ty(Ty) {
verify();
}
Access(Instruction *LocalI, Instruction *RemoteI, Optional<Value *> Content,
AccessKind Kind, Type *Ty)
: LocalI(LocalI), RemoteI(RemoteI), Content(Content), Kind(Kind),
Ty(Ty) {
Access(Instruction *LocalI, Instruction *RemoteI, int64_t Offset,
int64_t Size, Optional<Value *> Content, AccessKind Kind, Type *Ty)
: LocalI(LocalI), RemoteI(RemoteI), Content(Content), OAS(Offset, Size),
Kind(Kind), Ty(Ty) {
verify();
}
Access(const Access &Other) = default;
Access(const Access &&Other)
: LocalI(Other.LocalI), RemoteI(Other.RemoteI), Content(Other.Content),
Kind(Other.Kind), Ty(Other.Ty) {}
OAS(Other.OAS), Kind(Other.Kind), Ty(Other.Ty) {}

Access &operator=(const Access &Other) = default;
bool operator==(const Access &R) const {
return LocalI == R.LocalI && RemoteI == R.RemoteI &&
return LocalI == R.LocalI && RemoteI == R.RemoteI && OAS == R.OAS &&
Content == R.Content && Kind == R.Kind;
}
bool operator!=(const Access &R) const { return !(*this == R); }

Access &operator&=(const Access &R) {
assert(RemoteI == R.RemoteI && "Expected same instruction!");
Content =
AA::combineOptionalValuesInAAValueLatice(Content, R.Content, Ty);
assert(LocalI == R.LocalI && "Expected same instruction!");
Kind = AccessKind(Kind | R.Kind);
auto Before = OAS;
OAS &= R.OAS;
if (Before.isUnassigned() || Before == OAS) {
Content =
AA::combineOptionalValuesInAAValueLatice(Content, R.Content, Ty);
} else {
// Since the OAS information changed, set a conservative state -- drop
// the contents, and assume MayAccess rather than MustAccess.
Content.reset();
Kind = AccessKind(Kind | AK_MAY);
Kind = AccessKind(Kind & ~AK_MUST);
}
verify();
return *this;
}

Expand Down Expand Up @@ -5066,6 +5096,12 @@ struct AAPointerInfo : public AbstractAttribute {
/// determined.
Optional<Value *> getContent() const { return Content; }

/// Return the offset for this access.
int64_t getOffset() const { return OAS.Offset; }

/// Return the size for this access.
int64_t getSize() const { return OAS.Size; }

private:
/// The instruction responsible for the access with respect to the local
/// scope of the associated attribute.
Expand All @@ -5078,6 +5114,9 @@ struct AAPointerInfo : public AbstractAttribute {
/// cannot be determined.
Optional<Value *> Content;

/// The object accessed, in terms of an offset and size in bytes.
AA::OffsetAndSize OAS;

/// The access kind, e.g., READ, as bitset (could be more than one).
AccessKind Kind;

Expand Down Expand Up @@ -5113,7 +5152,7 @@ struct AAPointerInfo : public AbstractAttribute {
virtual bool forallInterferingAccesses(
Attributor &A, const AbstractAttribute &QueryingAA, Instruction &I,
function_ref<bool(const Access &, bool)> CB, bool &HasBeenWrittenTo,
AA::OffsetAndSize *OASPtr = nullptr) const = 0;
AA::OffsetAndSize &OAS) const = 0;

/// This function should return true if the type of the \p AA is AAPointerInfo
static bool classof(const AbstractAttribute *AA) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/IPO/Attributor.cpp
Expand Up @@ -456,7 +456,7 @@ static bool getPotentialCopiesOfMemoryValue(
auto &PI = A.getAAFor<AAPointerInfo>(QueryingAA, IRPosition::value(*Obj),
DepClassTy::NONE);
if (!PI.forallInterferingAccesses(A, QueryingAA, I, CheckAccess,
HasBeenWrittenTo, &OAS)) {
HasBeenWrittenTo, OAS)) {
LLVM_DEBUG(
dbgs()
<< "Failed to verify all interfering accesses for underlying object: "
Expand Down

0 comments on commit b756096

Please sign in to comment.