Refactor commoning of target specific load/store intrinsics in EarlyCSE

[ssijaric]

Summary

Introduce LoadSite and StoreSite to enable us to deal with target specific load/store intrinsics with characteristics similar to those of regular load and store instructions. Remove ParseMemoryInst from EarlyCSE, and replace it with LoadSite and StoreSite.

[ssijaric]

Ping

[reames]

Overall, heading in the right direction. I'm still not particularly happy about the complexity in EarlyCSE to handle target intrinsics at all, but this is definitely looking better than what's there currently.

Please update with context. The current diff is too hard to review in full. Some comments given, but they're really incomplete.

include/llvm/IR/LoadStoreSite.h

1 ↗

(On Diff #21905)

I might call this something like MemoryAccessSite.h or the like. Mild preference, don't bother changing this until other things are settled.

33 ↗

(On Diff #21905)

I might call this MemoryAccessBase, but that's a minor preference. My thought process is that it allows a logic extension for an AtomicRMWSite at some point in the future.

69 ↗

(On Diff #21905)

I'd wrap getInt in a helper function which makes the purpose clearer to the reader. The following seems cleaner and easier to read:

if (I.isTargetIntrinsic())

TTI->getTargetMemoryIntrinsicPointerOperand(

cast<IntrinsicInstTy>(Inst));
return cast<NativeInstr>(Inst)->getPointerOperand();

76 ↗

(On Diff #21905)

You've got this logic duplicated in a few places. Please common.

99 ↗

(On Diff #21905)

Having this here is sorta okay, but it really feels like this needs to live somewhere that applies easily to all intrinsics. We have the readonly/readnone attributes, why not just mark the underling intrinsics appropriately?

110 ↗

(On Diff #21905)

This shouldn't be necessary. If EarlyCSE doesn't know how to insert bitcasts, it really, really should.

146 ↗

(On Diff #21905)

This should probably be private.

lib/Analysis/TargetTransformInfo.cpp

269

What do you mean by "Atomic" here? AtomicRMW (the instruction?). Atomic (as in memory ordering?) The naming is very unclear.

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

539

Surely there are other intrinsics which read from memory? I really doubt the correctness of this code.

lib/Transforms/Scalar/EarlyCSE.cpp

567

a) naming, b) inverted conditional (bug!)

[reames]

include/llvm/Analysis/TargetTransformInfo.h

471

The naming of these functions is inconsistent. I'm fine with either scheme, but please be consistent.

lib/Transforms/Scalar/EarlyCSE.cpp

469

Volatile != Simple. Please use appropriate naming to match underling instruction.

482

Er, is this a separate bugfix? I don't understand where this is coming from w.r.t. the requested refactor.

510

It really feels like the functionality for whether an intrinsic reads from memory should live inside mayReadFromMemory.

545

Given this is testing whether a location matches not whether the instruction matches, I would like to see something which made that explicit.

[ssijaric]

Hi Philip,

Thanks for reviewing these changes! I'll upload a new patch with context.

include/llvm/Analysis/TargetTransformInfo.h

471

The inconsistency in naming is to indicate that isTargetIntrinsicLikeLoad may be invoked on any intrinsic instruction, whereas isTargetMemoryIntrinsic..... may be invoked on intrinsics that are either like loads or like stores. The asserts in TargetTransformInfo.cpp check for these properties.

I renamed these from isTargetMemoryIntrinsic* to isTargetIntrinsic* in the new patch. I kept the asserts for now. Maybe they should be renamed to something like isTargetLoadOrStoreIntrinsicAtomic instead?

Alternatively, we could do away with different query functions, and just have something like

MemoryIntrinsicInfo getTargetMemoryIntrinsic(IntrinsicInst*);

with overloaded operator bool(), which would return true if the MemoryIntrinsicInfo contains a supported intrinsic. The overhead would be that we would probably need to keep one MemoryIntrinsicInfo struct in LoadStoreSiteBase. This could perhaps avoid any inconsistencies due to having different query functions.

include/llvm/IR/LoadStoreSite.h

1 ↗

(On Diff #21905)

Yes, let's deal with renaming once other issues are addressed.

33 ↗

(On Diff #21905)

As per your previous suggestion, let's deal with renaming once other issues are addressed.

69 ↗

(On Diff #21905)

Done in the updated patch.

76 ↗

(On Diff #21905)

Commoned in the updated patch.

99 ↗

(On Diff #21905)

On AArch64 and ARM at least, store intrinsics are not marked as readnone by the front-end, and so mayReadFromMemory would always return true. For regular store instructions, mayReadFromMemory always returns false for non-atomic stores. This change allows the back-end to treat store intrinsics like regular stores.

I don't think it's legal to mark store intrinsics with readnone. When readnone is used on a function, then the function "does not write through any pointer arguments", according to the LLVM Language Reference Manual.

110 ↗

(On Diff #21905)

I put this here for completeness, in case some other optimization makes use of this API. For example, it would return false when checking "store i32 %val, i32* %ptr" and "load %val2 = load i16, i16* %ptr2".

This mirrors the check for

%0 = bitcast i32* %a to i8*
call void @llvm.aarch64.neon.st2.v4i32.p0i8(<4 x i32> %3, <4 x i32> %4, i8* %0)

and

%5 = bitcast i32* %a to i8*
%vld3 = call { <4 x i32>, <4 x i32>, <4 x i32> } @llvm.aarch64.neon.ld3.v4i32.p0i8(i8* %5)

While the check for non-intrinsic loads and stores is not necessary when using this API in EaryCSE, I don't think I should just return "true" unconditionally for regular loads and stores.

146 ↗

(On Diff #21905)

Changed in the updated patch.

lib/Analysis/TargetTransformInfo.cpp

269

This is supposed to check for memory ordering, similar to isAtomic() when invoked on regular loads and stores. The new name, isTargetIntrinsicAtomic(), is probably not any better. Is the comment in TargetTransformInfo.h enough?

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

539

There are, but only a few intrinsics are supported for now (the ones in isTargetIntrinsicLikeLoad(...) and isTargetIntrinsicLikeStore(...)). The assert makes sure that we are dealing with supported load/store intrinsics.

Perhaps, a better way would be to just have one query function for load/store intrinsics. Something like

MemoryIntrinsicInfo getTargetMemoryIntrinsic(IntrinsicInst *Inst);

which would return all the needed information inside the MemoryIntrinsicInfo struct? Then we could do away with separate APIs, which may be confusing. The overhead would be the inclusion of this struct in LoadStoreSiteBase.

lib/Transforms/Scalar/EarlyCSE.cpp

469

Originally, the check was

// Ignore volatile loads.
if (!LI->isSimple())

The check "if (MemInst.isVolatile())" does the same check underneath, at least for regular loads and stores. The name of MemInst.isVolatile() is wrong - it should have been MemInst.isSimple()).

The new check

if (!LS.isSimple()) restores the original check that was in place.

The comment , " Ignore volatile loads." should probably be " Ignore volatile and atomic loads".

482

Yes, this bug was pointed out by Hal, and could affect Power intrinsics from what I understand. I thought I'd include it here. If you prefer, I can do this under a seperate bug fix once the refactor is sorted out.

510

AArch64 store intrinsics are not marked as readnone, and mayReadFromMemory will always return true (unlike for regular non-atomic stores). So, we wouldn't get a chance to common intrinsic stores. It doesn't look to be legal to mark store intrinsics as readnone, according to the LLVM Language Reference Manual.

545

The previous change, isMatchingMemLoc, has a misleading name. It checks for both the memory operand and the matching id. This is what LoadStoreSiteBase::Match does as well. I renamed LoadStoreSiteBase::Match to LoadStoreSiteBase::MatchInstructionAndLocation in the new patch.

567

MemInst.isVolatile() is not correctly named - it checks for !isSimple() for regular loads and stores.

Previous to MemInst change, the check was

if (SI->isSimple())

The new change, if (SS.isSimple()), restores the original check.

[ssijaric]

A few changes to address Philip's comments for LoadSite, StoreSite and the target query functions. Uploading with full context.

[hfinkel]

include/llvm/Analysis/TargetTransformInfo.h

501

We have the IntrReadArgMem intrinsic property, how does this differ?

532

II->mayReadFromMemory()?

[ssijaric]

include/llvm/Analysis/TargetTransformInfo.h

501

From what I understand, IntrReadArgMem results in an intrinsic being annotated with the readonly attribute. However, the readonly attribute doesn't imply that the intrinsic has only one pointer argument.

532

I think I need to provide a better comment here. For regular, non-atomic stores, Instruction::mayReadFromMemory() returns false. This is not the case for calls unless they are marked with readnone, and store intrinsics are not marked with readnone.

[reames]

A few standalone comments before I go through the entire patch again.

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

539

I believe you've changed the name since the last version. Make sure the comments indicate that this only applies to target load and store intrinsics. I'm then fine with that.

lib/Transforms/Scalar/EarlyCSE.cpp

469

Update the comment please.

482

I would strongly prefer this be done separately.

510

You misunderstood my comment. I was suggesting that the *function* mayReadFromMemory should check whether the intrinsic is a target specific store and return false. I'm fine with this being a separate refactor, but this type of special case doesn't belong in a particular transform pass when the answer is the same for anyone who asks.

[reames]

Ok, I'm running out of comments to make. The EarlyCSE code is much improved here; thank you for taking the time to do this.

I'm not entirely thrilled with how complex the target interface is, but that can be addressed incrementally. What you proposing appears to be correct and is now clearly documented enough to not be confusing.

If you do last one last update with all the naming and small things addressed, I'll do one more run through for a final LGTM. I considered giving a conditional LGTM now, but there a few too many issues that were deferred. (In particular, separation of unrelated bug fixes and naming.) The next round should be fast though.

include/llvm/IR/LoadStoreSite.h

41 ↗

(On Diff #23125)

Leaving the member state unchanged after a call to setInstruction seems bug prone at best. Please either assert or clear the underlying data. (Looking through the calling code, I don't believe this is currently an issue. This is purely future proofing.)

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

544

I would prefer you not make the variable const rather than need a const_cast here.

[ssijaric]

Sorry for taking so long to get back to this. This update addresses Philip's comments.

[ssijaric]

Sorry for taking so long to get back to this. The updated patch is revised to address previous comments. Also moved LoadStoreSite.h to include/llvm/Analysis, since it makes use of TargetTransformInfo.

include/llvm/IR/LoadStoreSite.h

41 ↗

(On Diff #23125)

Addressed in the updated patch - the state is always updated.

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

544

Done in the updated patch.

588

I changed this a little, so that mayTargetIntrinsicReadFromMemory and mayTargetIntrinsicWriteToMemory apply to all intrinsics that are supported by the target (if they choose to do so).

The default implementation in TargetTransformInfoImplBase is:

bool mayTargetIntrinsicReadFromMemory(const IntrinsicInst *II) {

return !cast<CallInst>(II)->doesNotAccessMemory();

}

I also introduced bool mayReadFromMemory(const Instruction *I) and bool mayWriteToMemory(const Instruction *I) to TTI that may invoke mayTargetIntrinsicReadFromMemory and mayTargetIntrinsicWriteToMemory, respectively. This lets us do away with the kludgy check in EarlyCSE.

lib/Transforms/Scalar/EarlyCSE.cpp

511–513

Updated in the new patch.

524

Will be done in the updated patch - the bug will be addressed separately. The bug can only affect non-AArch64 users, of which there are none as far I can see.

550

Thanks for clarifying. Reworked in the updated patch - the checks are now done using TTI.mayReadFromMemory(Inst).

[ssijaric]

Fix a couple of function signatures.

[hfinkel]

Please post this patch with full context, see: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[ssijaric]

Upload with full context this time.

[reames]

One final round of comments. This looks really really close to ready to check in. Please make the requested changes (and nothing else) and I'll give it a LGTM once the patch is updated. Any further enhancements should go in separate follow on patches.

include/llvm/Analysis/LoadStoreSite.h

48

auto

100

This API feels really awkward. For now, I'll ask that you pull this function out of the helper class and make it a static function in EarlyCSE. That at least localizes the awkwardness to the place it's used.

123

Same as previous.

include/llvm/Analysis/TargetTransformInfo.h

501

It looks like you may have merged two patches by accident here?

504

Important detail that needs to be in this comment: not all load like target instructions will return true. This is correct because the optimizer must treat intrinsics calls conservatively. i.e. this is an optimization hook the target can hook into, not a correctness hook.

551

Why not just mark the intrinsics with readonly or readnone? Unless you have a very good reason, please do that.

561

This is the wrong place for this API. It should probably be on LoadSiteBase. It doesn't need to be part of TTI.

568

Same as above.

include/llvm/Analysis/TargetTransformInfoImpl.h

342

A slightly clearer way to phrase these assertions would be that the intrinsic passes either the LikeLoad or LikeStore predicate. This allows a target to override those two and not have to update the others to get a conservatively correct answer.

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

518

You should assert that the intrinsic is in either the load or store list. Otherwise, this could return an incorrect result for an unknown intrinsic.

Alternatively, it could be:
if intrinsic in [known list] return false;
else return true;

523

Same.

534

Submit separately please.

596

If you rephrase the assertions above, you can just call the existing check functions and remove all these comments. Please do so.

lib/Transforms/Scalar/EarlyCSE.cpp

312–315

Separable change. Please submit separately.

549

Ah, given where this is used, ignore my comment about putting this on LoadStoreBase. Making it a static function in EarlyCSE is better for the moment. Still not on TTI, but LSB isn't a good place either.

592

Use the assignment idiom as above.

[hfinkel]

I agree with Philip's comments (except, perhaps, for the one on which I've commented below).

include/llvm/Analysis/LoadStoreSite.h

100

Philip, why do you feel this is awkward? It seems that if the point of the abstraction is to hide the differentiation between regular load/stores and related target intrinsics. As such, it seems that this is an important part of the abstraction.

126

MatchInstructionAndLocation -> matchInstructionAndLocation

[reames]

include/llvm/Analysis/LoadStoreSite.h

100

Er, I've forgotten it's been so long since I looked at this. This was a minor comment and can be safely ignored. This can be addressed post commit if needed; I really don't want to block submission of this given how long it's been outstanding.

[ssijaric]

include/llvm/Analysis/LoadStoreSite.h

48

Will change to auto throughout in the updated patch.

100

I'll keep isCompatible in LoadStoreSite for now. I'll move matchInstructionAndLocation to EarlyCSE.

123

Will move it to EarlyCSE.

126

Will rename and move to EarlyCSE.

include/llvm/Analysis/TargetTransformInfo.h

501

Git format-patch added a bit of additional context - removing areInlineCompatible, and then adding it back in. The function areInlineCompatible is part of TTI already.

504

Will update in the new patch.

551

I did this mainly for consistency. I need mayTargetIntrinsicReadFromMemory to return false for store-like intrinsics (which can't have the readnone attribute set). So, I thought I'd introduce mayTargetIntrinsicWriteToMemory.

I'll remove mayTargetIntrinsicWriteToMemory in the updated patch, as load intrinsics coming from clang are already marked with the "readonly" attribute. (At least AArch64 ones are).

561

I put this function (and mayWriteToMemory) here so that we can use a single check in EarlyCSE, as in:

if (TTI.mayReadFromMemory(Inst))

...

Otherwise, if it's moved to LoadStoreBase, we have to check along the lines:

StoreSite S(Inst, TTI);
if (Inst->mayReadFromMemory() && !(StoreSite && !StoreSite.mayReadFromMemory())

I can move mayReadFromMemory(cons Instruction *I) to EarlyCSE?

568

Will move it to EarlyCSE.

include/llvm/Analysis/TargetTransformInfoImpl.h

342

I will remove the assert for isTargetIntrinsicAtomic, and call II->isAtomic() for the default implementation. For isTargetIntrinsicVolatile, I'll add the asserts if an intrinsic isn't like a load or a store, and return true otherwise to err on the conservative side.

I will keep asserts for getTargetIntrinsicPointerOperand, getOrCreateResultFromTargetIntrinsic and getTargetIntrinsicMatchingId, as these must be implemented by the target if they are called.

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

518

I will add a check for load/store intrinsic for isTargetIntrinsicVolatile. For isTargetIntrinsicAtomic, I'll return Inst->isAtomic() in the base TTI implementation.

523

Will change this in the updated patch.

534

Will change in the updated patch.

596

I'll add a check for volatility and atomicity in mayTargetIntrinsicReadFromMemory in the updated patch, and remove the comments. I'll remove mayTargetIntrinsicWriteToMemory altogether as load intrinsics are marked as readonly.

lib/Transforms/Scalar/EarlyCSE.cpp

312–315

Will do in the updated patch.

549

Will make it a static function in the updated patch.

592

Will fix in the updated patch.

[ssijaric]

Update to take into account Arnaud's bug fix.

[hfinkel]

include/llvm/Analysis/TargetTransformInfo.h

501

From what I understand, IntrReadArgMem results in an intrinsic being annotated with the readonly attribute. However, the readonly attribute doesn't imply that the intrinsic has only one pointer argument.

Not exactly. Such an intrinsic is read-only, but BasicAA also (automatically) learns the "arg mem" part of the property so that AA->getModRefBehavior can return FMRB_OnlyReadsArgumentPointees. Also, argmemonly is now a generic IR attribute that can be applied to any function.

518

If this is important, why not add function attributes for these?

[reames]

Resigning from a stale review (2016). Feel free to re-add if thread ever revived.

include/llvm/Analysis/LoadStoreSite.h

+//===- LoadStoreSite.h - Abstract Load and Store instructions ---*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines LoadSite and StoreSite classes that provide common
+// interface to deal with regular load and store instructions, as well target
+// specific intrinsic load/store instructions.
+//
+// Example use:
+// if (LoadSite LS = LoadSite(Inst, TTI)) {
+//   const Value *Ptr = LS.getPointerOperand();
+//   ....
+// }
+//
+#ifndef LLVM_IR_LOADSTORESITE_H
+#define LLVM_IR_LOADSTORESITE_H
+
+#include "llvm/ADT/PointerIntPair.h"
+#include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
+
+namespace llvm {
+
+template <typename Derived, typename NativeInstr, typename ValTy = Value,
+          typename InstrTy = Instruction,
+          typename IntrinsicInstTy = IntrinsicInst>
+class LoadStoreSiteBase {
+protected:
+  // The bool bit is true if I is holding a pointer to NativeInstr, and false
+  // if I is holding a pointer to IntrinsicInst.  When I is holding nullptr,
+  // the bool bit is ignored.
+  PointerIntPair<InstrTy *, 1, bool> I;
+  const TargetTransformInfo &TTI;
+
+private:
+  void setInstruction(ValTy *V) {
+    I.setPointer(nullptr);
+    I.setInt(false);
+    if (V == nullptr)
+      return;
+    if (auto *Inst = dyn_cast<NativeInstr>(V)) {
+      I.setPointer(Inst);
+      I.setInt(true);
+    } else if (auto *II = dyn_cast<IntrinsicInstTy>(V)) {
+      if (Derived::isTargetIntrinsicSupported(II, this->TTI))
+        I.setPointer(II);
+    }
+  }
+  bool isTargetIntrinsic() const {
+    assert(I.getPointer() && "Instruction not set");
+    return !I.getInt();
+  }
+
+public:
+  LoadStoreSiteBase(ValTy *V, const TargetTransformInfo &TTI)
+      : I(nullptr, false), TTI(TTI) {
+    setInstruction(V);
+  }
+
+  /// isSimple - true if the load/store instruction or intrinsic is not volatile
+  /// or atomic.
+  bool isSimple() const {
+    assert(I.getPointer() && "Instruction not set");
+    auto *Inst = I.getPointer();
+    if (isTargetIntrinsic())
+      return !TTI.isTargetIntrinsicAtomic(cast<IntrinsicInstTy>(Inst)) &&
+             !TTI.isTargetIntrinsicVolatile(cast<IntrinsicInstTy>(Inst));
+    return cast<NativeInstr>(Inst)->isSimple();
+  }
+
+  /// getPointerOperand - retrieve the memory location from a load/store
+  /// instruction or intrinsic.
+  ValTy *getPointerOperand() const {
+    assert(I.getPointer() && "Instruction not set");
+    auto *Inst = I.getPointer();
+    if (isTargetIntrinsic())
+      return TTI.getTargetIntrinsicPointerOperand(cast<IntrinsicInstTy>(Inst));
+    return cast<NativeInstr>(Inst)->getPointerOperand();
+  }
+
+  LoadStoreSiteBase &operator=(InstrTy *Inst) {
+    setInstruction(Inst);
+    return *this;
+  }
+
+  explicit operator bool() const { return I.getPointer() != nullptr; }
+  InstrTy *operator->() const { return I.getPointer(); }
+  InstrTy *getInstruction() const { return I.getPointer(); }
+
+  /// isCompatible - true if Other is a compatible load or store instruction.
+  /// Two load or store instructions are compatible if they are the same
+  /// type of instruction.  Given a load instruction, a compatible store
+  /// instruction is the one that is a mirror of the load instruction.
+  bool isCompatible(const Value *Other) const {
+    assert(I.getPointer() && "Instruction not set");
+    if (!isTargetIntrinsic()) {
+      if (auto *LI = dyn_cast<LoadInst>(Other))
+        return getPointerOperand()->getType() ==
+               LI->getPointerOperand()->getType();
+      else if (auto *SI = dyn_cast<StoreInst>(Other))
+        return getPointerOperand()->getType() ==
+               SI->getPointerOperand()->getType();
+      return false;
+    }
+    if (isa<IntrinsicInstTy>(Other)) {
+      assert(isa<IntrinsicInstTy>(I.getPointer()) && "Expecting intrinsic");
+      if (TTI.getTargetIntrinsicMatchingId(cast<IntrinsicInstTy>(Other)) ==
+          TTI.getTargetIntrinsicMatchingId(
+              cast<IntrinsicInstTy>(I.getPointer())))
+        return true;
+      return false;
+    }
+    return false;
+  }
+};
+
+class LoadSite : public LoadStoreSiteBase<LoadSite, LoadInst> {
+  using Base = LoadStoreSiteBase<LoadSite, LoadInst>;
+  friend class LoadStoreSiteBase<LoadSite, LoadInst>;
+  static bool isTargetIntrinsicSupported(const IntrinsicInst *II,
+                                         const TargetTransformInfo &TTI) {
+    return TTI.isTargetIntrinsicLikeLoad(II);
+  }
+
+public:
+  LoadSite(Value *V, const TargetTransformInfo &TTI) : Base(V, TTI) {}
+  LoadSite &operator=(Instruction *Inst) {
+    Base::operator=(Inst);
+    return *this;
+  }
+};
+
+class StoreSite : public LoadStoreSiteBase<StoreSite, StoreInst> {
+  using Base = LoadStoreSiteBase<StoreSite, StoreInst>;
+  friend class LoadStoreSiteBase<StoreSite, StoreInst>;
+  static bool isTargetIntrinsicSupported(const IntrinsicInst *II,
+                                         const TargetTransformInfo &TTI) {
+    return TTI.isTargetIntrinsicLikeStore(II);
+  }
+
+public:
+  StoreSite(Value *V, const TargetTransformInfo &TTI) : Base(V, TTI) {}
+  StoreSite &operator=(Instruction *Inst) {
+    Base::operator=(Inst);
+    return *this;
+  }
+};
+}
+#endif

include/llvm/Analysis/TargetTransformInfo.h

 class Function;
 class GlobalValue;
 class Loop;
 class PreservedAnalyses;
 class Type;
 class User;
 class Value;
 
-/// \brief Information about a load/store intrinsic defined by the target.
-struct MemIntrinsicInfo {
-  MemIntrinsicInfo()
-      : ReadMem(false), WriteMem(false), Vol(false), MatchingId(0),
-        NumMemRefs(0), PtrVal(nullptr) {}
-  bool ReadMem;
-  bool WriteMem;
-  bool Vol;
-  // Same Id is set by the target for corresponding load/store intrinsics.
-  unsigned short MatchingId;
-  int NumMemRefs;
-  Value *PtrVal;
-};
-
 /// \brief This pass provides access to the codegen interfaces that are needed
 /// for IR-level transformations.
 class TargetTransformInfo {
 public:
   /// \brief Construct a TTI object using a type implementing the \c Concept
   /// API below.
   ///
   /// This is used by targets to construct a TTI wrapping their target-specific
public:
   ///
   /// Pairwise:
   ///  (v0, v1, v2, v3)
   ///  ((v0+v1), (v2, v3), undef, undef)
   /// Split:
   ///  (v0, v1, v2, v3)
   ///  ((v0+v2), (v1+v3), undef, undef)
   int getReductionCost(unsigned Opcode, Type *Ty, bool IsPairwiseForm) const;
 
   /// \returns The cost of Intrinsic instructions.
   int getIntrinsicInstrCost(Intrinsic::ID ID, Type *RetTy,
                             ArrayRef<Type *> Tys) const;
 
   /// \returns The cost of Call instructions.
   int getCallInstrCost(Function *F, Type *RetTy, ArrayRef<Type *> Tys) const;
 
   /// \returns The number of pieces into which the provided type must be
public:
 
   /// \returns The cost, if any, of keeping values of the given types alive
   /// over a callsite.
   ///
   /// Some types may require the use of register classes that do not have
   /// any callee-saved registers, so would require a spill and fill.
   unsigned getCostOfKeepingLiveOverCall(ArrayRef<Type *> Tys) const;
 
-  /// \returns True if the intrinsic is a supported memory intrinsic.  Info
-  /// will contain additional information - whether the intrinsic may write
-  /// or read to memory, volatility and the pointer.  Info is undefined
-  /// if false is returned.
-  bool getTgtMemIntrinsic(IntrinsicInst *Inst, MemIntrinsicInfo &Info) const;
+  /// \returns True if the two functions have compatible attributes for inlining
+  /// purposes.
+  bool areInlineCompatible(const Function *Caller,
+                           const Function *Callee) const;
+
+  /// \returns True if the target intrinsic behaves like a load instruction.
+  /// Must have one memory operand.
+  /// Not all load-like target intrinsics have to return true.  In that case,
+  /// they will be terated like regular intrinsics.
+  bool isTargetIntrinsicLikeLoad(const IntrinsicInst *II) const;
+
+  /// \returns True if the target intrinsic behaves like a store instruction.
+  /// Must have one memory operand.
+  /// Not all store-like target intrinsics have to return true.  In that case,
+  /// they will be treated like regular intrinsics.
+  bool isTargetIntrinsicLikeStore(const IntrinsicInst *II) const;
+
+  /// \returns True if the target memory intrinsic results in atomic memory
+  /// access.
+  bool isTargetIntrinsicAtomic(const IntrinsicInst *II) const;
+
+  /// \returns True if the target memory intrinsic results in volatile memory
+  /// access.
+  bool isTargetIntrinsicVolatile(const IntrinsicInst *II) const;
 
   /// \returns A value which is the result of the given memory intrinsic.  New
   /// instructions may be created to extract the result from the given intrinsic
   /// memory operation.  Returns nullptr if the target cannot create a result
   /// from the given intrinsic.
-  Value *getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
+  Value *getOrCreateResultFromTargetIntrinsic(IntrinsicInst *II,
                                               Type *ExpectedType) const;
 
-  /// \returns True if the two functions have compatible attributes for inlining
-  /// purposes.
-  bool areInlineCompatible(const Function *Caller,
-                           const Function *Callee) const;
+  /// \returns A pointer operand of the target memory intrinsic.
+  Value *getTargetIntrinsicPointerOperand(const IntrinsicInst *II) const;
+
+  /// \returns The Id indicating compatibility with another target memory
+  /// intrinsic.
+  /// A matching Id for a store memory intrinsic is the same as the matching Id
+  /// for its mirror load intrinsic.
+  unsigned getTargetIntrinsicMatchingId(const IntrinsicInst *II) const;
+
+  /// \returns True if the target memory intrinsic may read memory.
+  /// Target may override intrinsics such as store intrinsics, which are deemed
+  /// to read from memory by default as they are not marked with the 'readnone'
+  /// attribute.
+  bool mayTargetIntrinsicReadFromMemory(const IntrinsicInst *II) const;
 
   /// @}
 
 private:
   /// \brief The abstract base class used to type erase specific TTI
   /// implementations.
   class Concept;
 
   /// \brief The template model for the base class which wraps a concrete
   /// implementation in a type erased interface.
   template <typename T> class Model;
 
   std::unique_ptr<Concept> TTIImpl;
 };
 
 class TargetTransformInfo::Concept {
 public:
   virtual ~Concept() = 0;
   virtual const DataLayout &getDataLayout() const = 0;
   virtual int getOperationCost(unsigned Opcode, Type *Ty, Type *OpTy) = 0;
   virtual int getGEPCost(Type *PointeeType, const Value *Ptr,
                          ArrayRef<const Value *> Operands) = 0;
   virtual int getCallCost(FunctionType *FTy, int NumArgs) = 0;
   virtual int getCallCost(const Function *F, int NumArgs) = 0;
   virtual int getCallCost(const Function *F,
                           ArrayRef<const Value *> Arguments) = 0;
   virtual int getIntrinsicCost(Intrinsic::ID IID, Type *RetTy,
                                ArrayRef<Type *> ParamTys) = 0;
   virtual int getIntrinsicCost(Intrinsic::ID IID, Type *RetTy,
                                ArrayRef<const Value *> Arguments) = 0;
   virtual int getUserCost(const User *U) = 0;
   virtual bool hasBranchDivergence() = 0;
virtual int getReductionCost(unsigned Opcode, Type *Ty,
                                bool IsPairwiseForm) = 0;
   virtual int getIntrinsicInstrCost(Intrinsic::ID ID, Type *RetTy,
                                     ArrayRef<Type *> Tys) = 0;
   virtual int getCallInstrCost(Function *F, Type *RetTy,
                                ArrayRef<Type *> Tys) = 0;
   virtual unsigned getNumberOfParts(Type *Tp) = 0;
   virtual int getAddressComputationCost(Type *Ty, bool IsComplex) = 0;
   virtual unsigned getCostOfKeepingLiveOverCall(ArrayRef<Type *> Tys) = 0;
-  virtual bool getTgtMemIntrinsic(IntrinsicInst *Inst,
-                                  MemIntrinsicInfo &Info) = 0;
-  virtual Value *getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
-                                                   Type *ExpectedType) = 0;
   virtual bool areInlineCompatible(const Function *Caller,
                                    const Function *Callee) const = 0;
+  virtual bool isTargetIntrinsicLikeLoad(const IntrinsicInst *II) = 0;
+  virtual bool isTargetIntrinsicLikeStore(const IntrinsicInst *II) = 0;
+  virtual bool isTargetIntrinsicAtomic(const IntrinsicInst *II) = 0;
+  virtual bool isTargetIntrinsicVolatile(const IntrinsicInst *II) = 0;
+  virtual Value *getOrCreateResultFromTargetIntrinsic(IntrinsicInst *II,
+                                                      Type *ExpectedType) = 0;
+  virtual Value *getTargetIntrinsicPointerOperand(const IntrinsicInst *II) = 0;
+  virtual unsigned getTargetIntrinsicMatchingId(const IntrinsicInst *II) = 0;
+  virtual bool mayTargetIntrinsicReadFromMemory(const IntrinsicInst *II) = 0;
 };
 
 template <typename T>
 class TargetTransformInfo::Model final : public TargetTransformInfo::Concept {
   T Impl;
 
 public:
   Model(T Impl) : Impl(std::move(Impl)) {}
unsigned getNumberOfParts(Type *Tp) override {
     return Impl.getNumberOfParts(Tp);
   }
   int getAddressComputationCost(Type *Ty, bool IsComplex) override {
     return Impl.getAddressComputationCost(Ty, IsComplex);
   }
   unsigned getCostOfKeepingLiveOverCall(ArrayRef<Type *> Tys) override {
     return Impl.getCostOfKeepingLiveOverCall(Tys);
   }
-  bool getTgtMemIntrinsic(IntrinsicInst *Inst,
-                          MemIntrinsicInfo &Info) override {
-    return Impl.getTgtMemIntrinsic(Inst, Info);
-  }
-  Value *getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
-                                           Type *ExpectedType) override {
-    return Impl.getOrCreateResultFromMemIntrinsic(Inst, ExpectedType);
-  }
   bool areInlineCompatible(const Function *Caller,
                            const Function *Callee) const override {
     return Impl.areInlineCompatible(Caller, Callee);
   }
+  bool isTargetIntrinsicLikeLoad(const IntrinsicInst *II) override {
+    return Impl.isTargetIntrinsicLikeLoad(II);
+  }
+  bool isTargetIntrinsicLikeStore(const IntrinsicInst *II) override {
+    return Impl.isTargetIntrinsicLikeStore(II);
+  }
+  bool isTargetIntrinsicAtomic(const IntrinsicInst *II) override {
+    return Impl.isTargetIntrinsicAtomic(II);
+  }
+  bool isTargetIntrinsicVolatile(const IntrinsicInst *II) override {
+    return Impl.isTargetIntrinsicVolatile(II);
+  }
+  Value *getOrCreateResultFromTargetIntrinsic(IntrinsicInst *II,
+                                              Type *ExpectedType) override {
+    return Impl.getOrCreateResultFromTargetIntrinsic(II, ExpectedType);
+  }
+  Value *getTargetIntrinsicPointerOperand(const IntrinsicInst *II) override {
+    return Impl.getTargetIntrinsicPointerOperand(II);
+  }
+  unsigned getTargetIntrinsicMatchingId(const IntrinsicInst *II) override {
+    return Impl.getTargetIntrinsicMatchingId(II);
+  }
+  bool mayTargetIntrinsicReadFromMemory(const IntrinsicInst *II) override {
+    return Impl.mayTargetIntrinsicReadFromMemory(II);
+  }
 };
 
 template <typename T>
 TargetTransformInfo::TargetTransformInfo(T Impl)
     : TTIImpl(new Model<T>(Impl)) {}
 
 /// \brief Analysis pass providing the \c TargetTransformInfo.
 ///

include/llvm/Analysis/TargetTransformInfoImpl.h

public:
   unsigned getNumberOfParts(Type *Tp) { return 0; }
 
   unsigned getAddressComputationCost(Type *Tp, bool) { return 0; }
 
   unsigned getReductionCost(unsigned, Type *, bool) { return 1; }
 
   unsigned getCostOfKeepingLiveOverCall(ArrayRef<Type *> Tys) { return 0; }
 
-  bool getTgtMemIntrinsic(IntrinsicInst *Inst, MemIntrinsicInfo &Info) {
-    return false;
-  }
-
-  Value *getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
-                                           Type *ExpectedType) {
-    return nullptr;
-  }
-
   bool areInlineCompatible(const Function *Caller,
                            const Function *Callee) const {
     return (Caller->getFnAttribute("target-cpu") ==
             Callee->getFnAttribute("target-cpu")) &&
            (Caller->getFnAttribute("target-features") ==
             Callee->getFnAttribute("target-features"));
   }
+
+  bool isTargetIntrinsicLikeLoad(const IntrinsicInst *II) { return false; }
+
+  bool isTargetIntrinsicLikeStore(const IntrinsicInst *II) { return false; }
+
+  bool isTargetIntrinsicAtomic(const IntrinsicInst *II) {
+    return II->isAtomic();
+  }
+
+  bool isTargetIntrinsicVolatile(const IntrinsicInst *II) {
+    assert((isTargetIntrinsicLikeLoad(II) || isTargetIntrinsicLikeStore(II)) &&
+           "Checking volatility for an intrinsic that is not a store or load-like");
+    return true;
+  }
+
+  Value *getOrCreateResultFromTargetIntrinsic(IntrinsicInst *II,
+                                              Type *ExpectedType) {
+    assert(false &&
+           "Target does not implement getOrCreateResultFromTargetIntrinsic");
+    return nullptr;
+  }
+
+  Value *getTargetIntrinsicPointerOperand(const IntrinsicInst *II) {
+    assert(false &&
+           "Target does not implement getTargetIntrinsicPointerOperand");
+    return nullptr;
+  }
+
+  unsigned getTargetIntrinsicMatchingId(const IntrinsicInst *II) {
+    assert(false &&
+           "Target does not implement getTargetIntrinsicMatchingId");
+    return 0;
+  }
+
+  bool mayTargetIntrinsicReadFromMemory(const IntrinsicInst *II) {
+    return !cast<CallInst>(II)->doesNotAccessMemory();
+  }
 };
 
 /// \brief CRTP base class for use as a mix-in that aids implementing
 /// a TargetTransformInfo-compatible class.
 template <typename T>
 class TargetTransformInfoImplCRTPBase : public TargetTransformInfoImplBase {
 private:
   typedef TargetTransformInfoImplBase BaseT;

lib/Analysis/TargetTransformInfo.cpp

int TargetTransformInfo::getVectorInstrCost(unsigned Opcode, Type *Val,
   return Cost;
 }
 
 int TargetTransformInfo::getMemoryOpCost(unsigned Opcode, Type *Src,
                                          unsigned Alignment,
                                          unsigned AddressSpace) const {
   int Cost = TTIImpl->getMemoryOpCost(Opcode, Src, Alignment, AddressSpace);
   assert(Cost >= 0 && "TTI should not produce negative costs!");
   return Cost;
 }
 
 int TargetTransformInfo::getMaskedMemoryOpCost(unsigned Opcode, Type *Src,
                                                unsigned Alignment,
                                                unsigned AddressSpace) const {
   int Cost =
       TTIImpl->getMaskedMemoryOpCost(Opcode, Src, Alignment, AddressSpace);
   assert(Cost >= 0 && "TTI should not produce negative costs!");
int TargetTransformInfo::getReductionCost(unsigned Opcode, Type *Ty,
   return Cost;
 }
 
 unsigned
 TargetTransformInfo::getCostOfKeepingLiveOverCall(ArrayRef<Type *> Tys) const {
   return TTIImpl->getCostOfKeepingLiveOverCall(Tys);
 }
 
-bool TargetTransformInfo::getTgtMemIntrinsic(IntrinsicInst *Inst,
-                                             MemIntrinsicInfo &Info) const {
-  return TTIImpl->getTgtMemIntrinsic(Inst, Info);
+bool TargetTransformInfo::areInlineCompatible(const Function *Caller,
+                                              const Function *Callee) const {
+  return TTIImpl->areInlineCompatible(Caller, Callee);
 }
 
-Value *TargetTransformInfo::getOrCreateResultFromMemIntrinsic(
-    IntrinsicInst *Inst, Type *ExpectedType) const {
-  return TTIImpl->getOrCreateResultFromMemIntrinsic(Inst, ExpectedType);
+bool TargetTransformInfo::isTargetIntrinsicLikeLoad(
+    const IntrinsicInst *II) const {
+  return TTIImpl->isTargetIntrinsicLikeLoad(II);
 }
 
-bool TargetTransformInfo::areInlineCompatible(const Function *Caller,
-                                              const Function *Callee) const {
-  return TTIImpl->areInlineCompatible(Caller, Callee);
+bool TargetTransformInfo::isTargetIntrinsicLikeStore(
+    const IntrinsicInst *II) const {
+  return TTIImpl->isTargetIntrinsicLikeStore(II);
+}
+
+bool TargetTransformInfo::isTargetIntrinsicAtomic(
+    const IntrinsicInst *II) const {
+  return TTIImpl->isTargetIntrinsicAtomic(II);
+}
+
+bool TargetTransformInfo::isTargetIntrinsicVolatile(
+    const IntrinsicInst *II) const {
+  return TTIImpl->isTargetIntrinsicVolatile(II);
+}
+
+Value *TargetTransformInfo::getOrCreateResultFromTargetIntrinsic(
+    IntrinsicInst *II, Type *ExpectedType) const {
+  return TTIImpl->getOrCreateResultFromTargetIntrinsic(II, ExpectedType);
+}
+
+Value *TargetTransformInfo::getTargetIntrinsicPointerOperand(
+    const IntrinsicInst *II) const {
+  return TTIImpl->getTargetIntrinsicPointerOperand(II);
+}
+
+unsigned TargetTransformInfo::getTargetIntrinsicMatchingId(
+    const IntrinsicInst *II) const {
+  return TTIImpl->getTargetIntrinsicMatchingId(II);
+}
+
+bool TargetTransformInfo::mayTargetIntrinsicReadFromMemory(
+    const IntrinsicInst *II) const {
+  return TTIImpl->mayTargetIntrinsicReadFromMemory(II);
 }
 
 TargetTransformInfo::Concept::~Concept() {}
 
 TargetIRAnalysis::TargetIRAnalysis() : TTICallback(&getDefaultTTI) {}
 
 TargetIRAnalysis::TargetIRAnalysis(
     std::function<Result(const Function &)> TTICallback)

lib/Target/AArch64/AArch64TargetTransformInfo.h

public:
 
   int getMemoryOpCost(unsigned Opcode, Type *Src, unsigned Alignment,
                       unsigned AddressSpace);
 
   int getCostOfKeepingLiveOverCall(ArrayRef<Type *> Tys);
 
   void getUnrollingPreferences(Loop *L, TTI::UnrollingPreferences &UP);
 
-  Value *getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
-                                           Type *ExpectedType);
-
-  bool getTgtMemIntrinsic(IntrinsicInst *Inst, MemIntrinsicInfo &Info);
-
   int getInterleavedMemoryOpCost(unsigned Opcode, Type *VecTy, unsigned Factor,
                                  ArrayRef<unsigned> Indices, unsigned Alignment,
                                  unsigned AddressSpace);
+
+  bool isTargetIntrinsicLikeLoad(const IntrinsicInst *II);
+  bool isTargetIntrinsicLikeStore(const IntrinsicInst *II);
+  bool isTargetIntrinsicVolatile(const IntrinsicInst *II);
+  Value *getOrCreateResultFromTargetIntrinsic(IntrinsicInst *Inst,
+                                              Type *ExpectedType);
+  Value *getTargetIntrinsicPointerOperand(const IntrinsicInst *II);
+  unsigned getTargetIntrinsicMatchingId(const IntrinsicInst *II);
+  bool mayTargetIntrinsicReadFromMemory(const IntrinsicInst *II);
   /// @}
 };
 
 } // end namespace llvm
 
 #endif

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

void AArch64TTIImpl::getUnrollingPreferences(Loop *L,
   // a larger threshold to unroll more loops.
   if (L->getLoopDepth() > 1)
     UP.PartialThreshold *= 2;
 
   // Disable partial & runtime unrolling on -Os.
   UP.PartialOptSizeThreshold = 0;
 }
 
-Value *AArch64TTIImpl::getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
-                                                         Type *ExpectedType) {
+bool AArch64TTIImpl::isTargetIntrinsicLikeLoad(const IntrinsicInst *Inst) {
+  switch (Inst->getIntrinsicID()) {
+  default:
+    return false;
+  case Intrinsic::aarch64_neon_ld2:
+  case Intrinsic::aarch64_neon_ld3:
+  case Intrinsic::aarch64_neon_ld4:
+    return true;
+  }
+}
+
+bool AArch64TTIImpl::isTargetIntrinsicLikeStore(const IntrinsicInst *Inst) {
+  switch (Inst->getIntrinsicID()) {
+  default:
+    return false;
+  case Intrinsic::aarch64_neon_st2:
+  case Intrinsic::aarch64_neon_st3:
+  case Intrinsic::aarch64_neon_st4:
+    return true;
+  }
+}
+
+// AArch64 load/store intrinsics are not volatile.
+bool AArch64TTIImpl::isTargetIntrinsicVolatile(const IntrinsicInst *Inst) {
+  assert((isTargetIntrinsicLikeLoad(Inst) || isTargetIntrinsicLikeStore(Inst))
+          && "Inst is not a supported target load or store intrinsic.");
+  return false;
+}
+
+Value *
+AArch64TTIImpl::getOrCreateResultFromTargetIntrinsic(IntrinsicInst *Inst,
   switch (Inst->getIntrinsicID()) {
   default:
     return nullptr;
   case Intrinsic::aarch64_neon_st2:
   case Intrinsic::aarch64_neon_st3:
   case Intrinsic::aarch64_neon_st4: {
     // Create a struct type
     StructType *ST = dyn_cast<StructType>(ExpectedType);
     if (!ST)
       return nullptr;
     unsigned NumElts = Inst->getNumArgOperands() - 1;
     if (ST->getNumElements() != NumElts)
       return nullptr;
     for (unsigned i = 0, e = NumElts; i != e; ++i) {
       if (Inst->getArgOperand(i)->getType() != ST->getElementType(i))
         return nullptr;
     }
     Value *Res = UndefValue::get(ExpectedType);
     IRBuilder<> Builder(Inst);
     for (unsigned i = 0, e = NumElts; i != e; ++i) {
       Value *L = Inst->getArgOperand(i);
       Res = Builder.CreateInsertValue(Res, L, i);
     }
     return Res;
   }
   case Intrinsic::aarch64_neon_ld2:
   case Intrinsic::aarch64_neon_ld3:
   case Intrinsic::aarch64_neon_ld4:
     if (Inst->getType() == ExpectedType)
       return Inst;
     return nullptr;
   }
 }
 
-bool AArch64TTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,
-                                        MemIntrinsicInfo &Info) {
-  switch (Inst->getIntrinsicID()) {
-  default:
-    break;
-  case Intrinsic::aarch64_neon_ld2:
-  case Intrinsic::aarch64_neon_ld3:
-  case Intrinsic::aarch64_neon_ld4:
-    Info.ReadMem = true;
-    Info.WriteMem = false;
-    Info.Vol = false;
-    Info.NumMemRefs = 1;
-    Info.PtrVal = Inst->getArgOperand(0);
-    break;
-  case Intrinsic::aarch64_neon_st2:
-  case Intrinsic::aarch64_neon_st3:
-  case Intrinsic::aarch64_neon_st4:
-    Info.ReadMem = false;
-    Info.WriteMem = true;
-    Info.Vol = false;
-    Info.NumMemRefs = 1;
-    Info.PtrVal = Inst->getArgOperand(Inst->getNumArgOperands() - 1);
-    break;
-  }
+Value *
+AArch64TTIImpl::getTargetIntrinsicPointerOperand(const IntrinsicInst *Inst) {
+  if (isTargetIntrinsicLikeLoad(Inst))
+    return Inst->getArgOperand(0);
+  assert(isTargetIntrinsicLikeStore(Inst) &&
+         "Expecting a target load or store intrinsic.");
+  return Inst->getArgOperand(Inst->getNumArgOperands() - 1);
 
+unsigned
+AArch64TTIImpl::getTargetIntrinsicMatchingId(const IntrinsicInst *Inst) {
   switch (Inst->getIntrinsicID()) {
   default:
-    return false;
+    assert(
+        false &&
+        "Calling AArch64TTIImpl::getMatchingId on an unsupported intrinsic.");
   case Intrinsic::aarch64_neon_ld2:
   case Intrinsic::aarch64_neon_st2:
-    Info.MatchingId = VECTOR_LDST_TWO_ELEMENTS;
-    break;
+    return VECTOR_LDST_TWO_ELEMENTS;
   case Intrinsic::aarch64_neon_ld3:
   case Intrinsic::aarch64_neon_st3:
-    Info.MatchingId = VECTOR_LDST_THREE_ELEMENTS;
-    break;
+    return VECTOR_LDST_THREE_ELEMENTS;
   case Intrinsic::aarch64_neon_ld4:
   case Intrinsic::aarch64_neon_st4:
-    Info.MatchingId = VECTOR_LDST_FOUR_ELEMENTS;
-    break;
+    return VECTOR_LDST_FOUR_ELEMENTS;
   }
-  return true;
+}
+
+// Store intrinsics are not marked with "readnone", and so mayReadFromMemory
+// will be true for store intrinsics.  This function will treat supported
+// non-volatile store intrinsics the same way as regular stores are treated -
+// not reading from memory.
+bool AArch64TTIImpl::mayTargetIntrinsicReadFromMemory(
+    const IntrinsicInst *Inst) {
+  if (isTargetIntrinsicLikeStore(Inst) &&
+      !isTargetIntrinsicVolatile(Inst) && !isTargetIntrinsicAtomic(Inst))
+    return false;
+  return BaseT::mayTargetIntrinsicReadFromMemory(Inst);
 }

lib/Transforms/Scalar/EarlyCSE.cpp

 
 #include "llvm/Transforms/Scalar/EarlyCSE.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/ScopedHashTable.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/GlobalsModRef.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/InstructionSimplify.h"
+#include "llvm/Analysis/LoadStoreSite.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/Pass.h"

 
 bool DenseMapInfo<CallValue>::isEqual(CallValue LHS, CallValue RHS) {
   Instruction *LHSI = LHS.Inst, *RHSI = RHS.Inst;
   if (LHS.isSentinel() || RHS.isSentinel())
     return LHSI == RHSI;
   return LHSI->isIdenticalTo(RHSI);
 }
 
+/// matchInstructionAndLocation - true if Other is a compatible load or store
+/// instruction, reading from, or writing to the same memory location.
+template<typename U, typename V>
+static bool matchInstructionAndLocation(const LoadStoreSiteBase<U, V> &Inst1,
+                                        const LoadStoreSiteBase<U, V> &Inst2) {
+  return Inst1.getPointerOperand() == Inst2.getPointerOperand() &&
+         Inst1.isCompatible(Inst2.getInstruction());
+}
+
+/// mayReadFromMemory - true if Inst may read memory.  This can override
+/// store intrinsics, which return true by default as they are not marked
+/// with the 'readnone' attribute.
+static bool mayReadFromMemory(const Instruction *Inst,
+                              const TargetTransformInfo &TTI) {
+  if (auto *II = dyn_cast<IntrinsicInst>(Inst))
+    return TTI.mayTargetIntrinsicReadFromMemory(II);
+  return Inst->mayReadFromMemory();
+}
+
 //===----------------------------------------------------------------------===//
 // EarlyCSE implementation
 //===----------------------------------------------------------------------===//
 
 namespace {
 /// \brief A simple and fast domtree-based CSE pass.
 ///
 /// This pass does a simple depth-first walk over the dominator tree,
public:
 
   /// \brief A scoped hash table of the current values of loads.
   ///
   /// This allows us to get efficient access to dominating loads when we have
   /// a fully redundant load.  In addition to the most recent load, we keep
   /// track of a generation count of the read, which is compared against the
   /// current generation count.  The current generation count is incremented
   /// after every possibly writing memory operation, which ensures that we only
   /// CSE loads with other loads that have no intervening store.
-  struct LoadValue {
-    Value *data;
-    unsigned generation;
-    int matchingId;
-    LoadValue() : data(nullptr), generation(0), matchingId(-1) {}
-    LoadValue(Value *data, unsigned generation, unsigned matchingId)
-        : data(data), generation(generation), matchingId(matchingId) {}
-  };
-  typedef RecyclingAllocator<BumpPtrAllocator,
-                             ScopedHashTableVal<Value *, LoadValue>>
+  typedef RecyclingAllocator<
+      BumpPtrAllocator,
+      ScopedHashTableVal<Value *, std::pair<Value *, unsigned>>>
       LoadMapAllocator;
-  typedef ScopedHashTable<Value *, LoadValue, DenseMapInfo<Value *>,
-                          LoadMapAllocator> LoadHTType;
+  typedef ScopedHashTable<Value *, std::pair<Value *, unsigned>,
+                          DenseMapInfo<Value *>, LoadMapAllocator> LoadHTType;
   LoadHTType AvailableLoads;
 
   /// \brief A scoped hash table of the current values of read-only call
   /// values.
   ///
   /// It uses the same generation count as loads.
   typedef ScopedHashTable<CallValue, std::pair<Value *, unsigned>> CallHTType;
   CallHTType AvailableCalls;
private:
     unsigned ChildGeneration;
     DomTreeNode *Node;
     DomTreeNode::iterator ChildIter;
     DomTreeNode::iterator EndIter;
     NodeScope Scopes;
     bool Processed;
   };
 
-  /// \brief Wrapper class to handle memory instructions, including loads,
-  /// stores and intrinsic loads and stores defined by the target.
-  class ParseMemoryInst {
-  public:
-    ParseMemoryInst(Instruction *Inst, const TargetTransformInfo &TTI)
-        : Load(false), Store(false), Vol(false), MayReadFromMemory(false),
-          MayWriteToMemory(false), MatchingId(-1), Ptr(nullptr) {
-      MayReadFromMemory = Inst->mayReadFromMemory();
-      MayWriteToMemory = Inst->mayWriteToMemory();
-      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
-        MemIntrinsicInfo Info;
-        if (!TTI.getTgtMemIntrinsic(II, Info))
-          return;
-        if (Info.NumMemRefs == 1) {
-          Store = Info.WriteMem;
-          Load = Info.ReadMem;
-          MatchingId = Info.MatchingId;
-          MayReadFromMemory = Info.ReadMem;
-          MayWriteToMemory = Info.WriteMem;
-          Vol = Info.Vol;
-          Ptr = Info.PtrVal;
-        }
-      } else if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
-        Load = true;
-        Vol = !LI->isSimple();
-        Ptr = LI->getPointerOperand();
-      } else if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
-        Store = true;
-        Vol = !SI->isSimple();
-        Ptr = SI->getPointerOperand();
-      }
-    }
-    bool isLoad() const { return Load; }
-    bool isStore() const { return Store; }
-    bool isVolatile() const { return Vol; }
-    bool isMatchingMemLoc(const ParseMemoryInst &Inst) const {
-      return Ptr == Inst.Ptr && MatchingId == Inst.MatchingId;
-    }
-    bool isValid() const { return Ptr != nullptr; }
-    int getMatchingId() const { return MatchingId; }
-    Value *getPtr() const { return Ptr; }
-    bool mayReadFromMemory() const { return MayReadFromMemory; }
-    bool mayWriteToMemory() const { return MayWriteToMemory; }
-
-  private:
-    bool Load;
-    bool Store;
-    bool Vol;
-    bool MayReadFromMemory;
-    bool MayWriteToMemory;
-    // For regular (non-intrinsic) loads/stores, this is set to -1. For
-    // intrinsic loads/stores, the id is retrieved from the corresponding
-    // field in the MemIntrinsicInfo structure.  That field contains
-    // non-negative values only.
-    int MatchingId;
-    Value *Ptr;
-  };
-
   bool processNode(DomTreeNode *Node);
 
   Value *getOrCreateResult(Value *Inst, Type *ExpectedType) const {
     if (LoadInst *LI = dyn_cast<LoadInst>(Inst))
       return LI;
     else if (StoreInst *SI = dyn_cast<StoreInst>(Inst))
       return SI->getValueOperand();
     assert(isa<IntrinsicInst>(Inst) && "Instruction not supported");
-    return TTI.getOrCreateResultFromMemIntrinsic(cast<IntrinsicInst>(Inst),
+    return TTI.getOrCreateResultFromTargetIntrinsic(cast<IntrinsicInst>(Inst),
                                                     ExpectedType);
   }
 };
 }
 
 bool EarlyCSE::processNode(DomTreeNode *Node) {
   BasicBlock *BB = Node->getBlock();
 
   // If this block has a single predecessor, then the predecessor is the parent
if (auto *BI = dyn_cast<BranchInst>(Pred->getTerminator()))
             replaceDominatedUsesWith(CondInst, ConditionalConstant, DT,
                                      BasicBlockEdge(Pred, BB));
           }
 
   /// LastStore - Keep track of the last non-volatile store that we saw... for
   /// as long as there in no instruction that reads memory.  If we see a store
   /// to the same location, we delete the dead store.  This zaps trivial dead
   /// stores which can occur in bitfield code among other things.
-  Instruction *LastStore = nullptr;
-
+  StoreSite LastStore(nullptr, TTI);
   bool Changed = false;
   const DataLayout &DL = BB->getModule()->getDataLayout();
 
   // See if any instructions in the block can be eliminated.  If so, do it.  If
   // not, add them to AvailableValues.
   for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E;) {
     Instruction *Inst = I++;
 
     // Dead instructions should just be removed.
     if (isInstructionTriviallyDead(Inst, &TLI)) {
       DEBUG(dbgs() << "EarlyCSE DCE: " << *Inst << '\n');
       Inst->eraseFromParent();
       Changed = true;
       ++NumSimplify;
       continue;
     }
 
     // Skip assume intrinsics, they don't really have side effects (although
     // they're marked as such to ensure preservation of control dependencies),
     // and this pass will not disturb any of the assumption's control
     // dependencies.
     if (match(Inst, m_Intrinsic<Intrinsic::assume>())) {
       DEBUG(dbgs() << "EarlyCSE skipping assumption: " << *Inst << '\n');
       continue;
     }
 
     // If the instruction can be simplified (e.g. X+0 = X) then replace it with
     // its simpler value.
     if (Value *V = SimplifyInstruction(Inst, DL, &TLI, &DT, &AC)) {
       DEBUG(dbgs() << "EarlyCSE Simplify: " << *Inst << "  to: " << *V << '\n');
       Inst->replaceAllUsesWith(V);
       Inst->eraseFromParent();
       Changed = true;
       ++NumSimplify;
       continue;
if (SimpleValue::canHandle(Inst)) {
         continue;
       }
 
       // Otherwise, just remember that this value is available.
       AvailableValues.insert(Inst, Inst);
       continue;
     }
 
-    ParseMemoryInst MemInst(Inst, TTI);
     // If this is a non-volatile load, process it.
-    if (MemInst.isValid() && MemInst.isLoad()) {
-      // Ignore volatile loads.
-      if (MemInst.isVolatile()) {
+    if (LoadSite LS = LoadSite(Inst, TTI)) {
+      // Ignore volatile and atomic loads.
+      if (!LS.isSimple()) {
         LastStore = nullptr;
         // Don't CSE across synchronization boundaries.
         if (Inst->mayWriteToMemory())
           ++CurrentGeneration;
         continue;
       }
 
       // If we have an available version of this load, and if it is the right
       // generation, replace this instruction.
-      LoadValue InVal = AvailableLoads.lookup(MemInst.getPtr());
-      if (InVal.data != nullptr && InVal.generation == CurrentGeneration &&
-          InVal.matchingId == MemInst.getMatchingId()) {
-        Value *Op = getOrCreateResult(InVal.data, Inst->getType());
+      std::pair<Value *, unsigned> InVal =
+          AvailableLoads.lookup(LS.getPointerOperand());
+      if (InVal.first != nullptr && InVal.second == CurrentGeneration &&
+          LS.isCompatible(InVal.first)) {
+        Value *Op = getOrCreateResult(InVal.first, Inst->getType());
         if (Op != nullptr) {
           DEBUG(dbgs() << "EarlyCSE CSE LOAD: " << *Inst
-                       << "  to: " << *InVal.data << '\n');
+                       << "  to: " << *InVal.first << '\n');
           if (!Inst->use_empty())
             Inst->replaceAllUsesWith(Op);
           Inst->eraseFromParent();
           Changed = true;
           ++NumCSELoad;
           continue;
         }
       }
 
       // Otherwise, remember that we have this instruction.
       AvailableLoads.insert(
-          MemInst.getPtr(),
-          LoadValue(Inst, CurrentGeneration, MemInst.getMatchingId()));
+          LS.getPointerOperand(),
+          std::pair<Value *, unsigned>(Inst, CurrentGeneration));
       LastStore = nullptr;
       continue;
     }
 
     // If this instruction may read from memory, forget LastStore.
-    // Load/store intrinsics will indicate both a read and a write to
-    // memory.  The target may override this (e.g. so that a store intrinsic
-    // does not read  from memory, and thus will be treated the same as a
-    // regular store for commoning purposes).
-    if (Inst->mayReadFromMemory() &&
-        !(MemInst.isValid() && !MemInst.mayReadFromMemory()))
+    if (mayReadFromMemory(Inst, TTI))
       LastStore = nullptr;
 
     // If this is a read-only call, process it.
     if (CallValue::canHandle(Inst)) {
       // If we have an available version of this call, and if it is the right
       // generation, replace this instruction.
       std::pair<Value *, unsigned> InVal = AvailableCalls.lookup(Inst);
       if (InVal.first != nullptr && InVal.second == CurrentGeneration) {
         DEBUG(dbgs() << "EarlyCSE CSE CALL: " << *Inst
                      << "  to: " << *InVal.first << '\n');
         if (!Inst->use_empty())
           Inst->replaceAllUsesWith(InVal.first);
         Inst->eraseFromParent();
         Changed = true;
         ++NumCSECall;
         continue;
       }
 
       // Otherwise, remember that we have this instruction.
       AvailableCalls.insert(
           Inst, std::pair<Value *, unsigned>(Inst, CurrentGeneration));
       continue;
     }
 
     // A release fence requires that all stores complete before it, but does
     // not prevent the reordering of following loads 'before' the fence.  As a
     // result, we don't need to consider it as writing to memory and don't need
     // to advance the generation.  We do need to prevent DSE across the fence,
     // but that's handled above.
     if (FenceInst *FI = dyn_cast<FenceInst>(Inst))
       if (FI->getOrdering() == Release) {
-        assert(Inst->mayReadFromMemory() && "relied on to prevent DSE above");
+        assert(mayReadFromMemory(Inst, TTI) && "relied on to prevent DSE above");
         continue;
       }
 
     // Okay, this isn't something we can CSE at all.  Check to see if it is
     // something that could modify memory.  If so, our available memory values
     // cannot be used so bump the generation count.
     if (Inst->mayWriteToMemory()) {
       ++CurrentGeneration;
 
-      if (MemInst.isValid() && MemInst.isStore()) {
+      if (StoreSite SS = StoreSite(Inst, TTI)) {
         // We do a trivial form of DSE if there are two stores to the same
         // location with no intervening loads.  Delete the earlier store.
         if (LastStore) {
-          ParseMemoryInst LastStoreMemInst(LastStore, TTI);
-          if (LastStoreMemInst.isMatchingMemLoc(MemInst)) {
-            DEBUG(dbgs() << "EarlyCSE DEAD STORE: " << *LastStore
-                         << "  due to: " << *Inst << '\n');
+          if (matchInstructionAndLocation(LastStore, SS)) {
+            DEBUG(dbgs() << "EarlyCSE DEAD STORE: "
+                         << *LastStore.getInstruction() << "  due to: " << *Inst
+                         << '\n');
             LastStore->eraseFromParent();
             Changed = true;
             ++NumDSE;
             LastStore = nullptr;
           }
           // fallthrough - we can exploit information about this store
         }
 
         // Okay, we just invalidated anything we knew about loaded values.  Try
         // to salvage *something* by remembering that the stored value is a live
         // version of the pointer.  It is safe to forward from volatile stores
         // to non-volatile loads, so we don't have to check for volatility of
         // the store.
-        AvailableLoads.insert(
-            MemInst.getPtr(),
-            LoadValue(Inst, CurrentGeneration, MemInst.getMatchingId()));
+        AvailableLoads.insert(SS.getPointerOperand(),
+                              std::pair<Value *, unsigned>(SS.getInstruction(),
+                                                           CurrentGeneration));
 
         // Remember that this was the last store we saw for DSE.
-        if (!MemInst.isVolatile())
+        if (SS.isSimple())
           LastStore = Inst;
       }
     }
   }
 
   return Changed;
 }