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

2

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

34

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.

70

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();

77

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

100

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?

111

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

147

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

2

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

34

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

70

Done in the updated patch.

77

Commoned in the updated patch.

100

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.

111

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.

147

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

467

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

498

II->mayReadFromMemory()?

[ssijaric]

include/llvm/Analysis/TargetTransformInfo.h

467

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.

498

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

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

495

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

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

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

495

Done in the updated patch.

539

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

469

Updated in the new patch.

482

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.

510

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

47 ↗

(On Diff #32266)

auto

99 ↗

(On Diff #32266)

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.

122 ↗

(On Diff #32266)

Same as previous.

include/llvm/Analysis/TargetTransformInfo.h

467

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

470

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.

516

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

526

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

533

Same as above.

include/llvm/Analysis/TargetTransformInfoImpl.h

353

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

466

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;

471

Same.

485

Submit separately please.

547

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

294

Separable change. Please submit separately.

509–510

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.

542

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

99 ↗

(On Diff #32266)

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.

125 ↗

(On Diff #32266)

MatchInstructionAndLocation -> matchInstructionAndLocation

[reames]

include/llvm/Analysis/LoadStoreSite.h

99 ↗

(On Diff #32266)

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

47 ↗

(On Diff #32266)

Will change to auto throughout in the updated patch.

99 ↗

(On Diff #32266)

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

122 ↗

(On Diff #32266)

Will move it to EarlyCSE.

125 ↗

(On Diff #32266)

Will rename and move to EarlyCSE.

include/llvm/Analysis/TargetTransformInfo.h

467

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.

470

Will update in the new patch.

516

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).

526

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?

533

Will move it to EarlyCSE.

include/llvm/Analysis/TargetTransformInfoImpl.h

353

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

466

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

471

Will change this in the updated patch.

485

Will change in the updated patch.

547

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

294

Will do in the updated patch.

509–510

Will make it a static function in the updated patch.

542

Will fix in the updated patch.

[ssijaric]

Update to take into account Arnaud's bug fix.

[hfinkel]

include/llvm/Analysis/TargetTransformInfo.h

467

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.

484

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/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:
 
   /// \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 target intrinsic behaves like a load instruction.
+  /// Must have one memory operand.
+  bool isTargetIntrinsicLikeLoad(const IntrinsicInst *II) const;
+
+  /// \returns True if the target intrinsic behaves like a store instruction.
+  /// Must have one memory operand.
+  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,
-                                           Type *ExpectedType) const;
+  Value *getOrCreateResultFromTargetIntrinsic(IntrinsicInst *II,
+                                              const Type *ExpectedType) 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.
+  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 unsigned getOperationCost(unsigned Opcode, Type *Ty, Type *OpTy) = 0;
   virtual unsigned getGEPCost(const Value *Ptr,
                               ArrayRef<const Value *> Operands) = 0;
   virtual unsigned getCallCost(FunctionType *FTy, int NumArgs) = 0;
   virtual unsigned getCallCost(const Function *F, int NumArgs) = 0;
   virtual unsigned getCallCost(const Function *F,
                                ArrayRef<const Value *> Arguments) = 0;
   virtual unsigned getIntrinsicCost(Intrinsic::ID IID, Type *RetTy,
                                     ArrayRef<Type *> ParamTys) = 0;
   virtual unsigned getIntrinsicCost(Intrinsic::ID IID, Type *RetTy,
                                     ArrayRef<const Value *> Arguments) = 0;
   virtual unsigned getUserCost(const User *U) = 0;
   virtual bool hasBranchDivergence() = 0;
   virtual bool isLoweredToCall(const Function *F) = 0;
   virtual void getUnrollingPreferences(Loop *L, UnrollingPreferences &UP) = 0;
   virtual bool isLegalAddImmediate(int64_t Imm) = 0;
   virtual bool isLegalICmpImmediate(int64_t Imm) = 0;
   virtual bool isLegalAddressingMode(Type *Ty, GlobalValue *BaseGV,
                                      int64_t BaseOffset, bool HasBaseReg,
                                      int64_t Scale) = 0;
   virtual bool isLegalMaskedStore(Type *DataType, int Consecutive) = 0;
   virtual bool isLegalMaskedLoad(Type *DataType, int Consecutive) = 0;
   virtual int getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
                                    int64_t BaseOffset, bool HasBaseReg,
                                    int64_t Scale) = 0;
virtual unsigned getReductionCost(unsigned Opcode, Type *Ty,
                                     bool IsPairwiseForm) = 0;
   virtual unsigned getIntrinsicInstrCost(Intrinsic::ID ID, Type *RetTy,
                                          ArrayRef<Type *> Tys) = 0;
   virtual unsigned getCallInstrCost(Function *F, Type *RetTy,
                                     ArrayRef<Type *> Tys) = 0;
   virtual unsigned getNumberOfParts(Type *Tp) = 0;
   virtual unsigned 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 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,
+                                       const 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);
   }
   unsigned 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 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,
+                                       const 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

 #define LLVM_ANALYSIS_TARGETTRANSFORMINFOIMPL_H
 
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/Type.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
 
 /// \brief Base class for use as a mix-in that aids implementing
 /// a TargetTransformInfo-compatible class.
 class TargetTransformInfoImplBase {
 protected:
   typedef TargetTransformInfo TTI;
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;
+  bool isTargetIntrinsicLikeLoad(const IntrinsicInst *II) { return false; }
+
+  bool isTargetIntrinsicLikeStore(const IntrinsicInst *II) { return false; }
+
+  bool isTargetIntrinsicAtomic(const IntrinsicInst *II) {
+    assert(false &&
+           "Target does not support querying of target memory intrinsics.");
+    return true;
+  }
+
+  bool isTargetIntrinsicVolatile(const IntrinsicInst *II) {
+    assert(false &&
+           "Target does not support querying of target memory intrinsics.");
+    return true;
+  }
+
+  Value *getOrCreateResultFromTargetIntrinsic(IntrinsicInst *II,
+                                              const Type *ExpectedType) {
+    assert(false &&
+           "Target does not support querying of target memory intrinsics.");
+    return nullptr;
   }
 
-  Value *getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
-                                           Type *ExpectedType) {
+  Value *getTargetIntrinsicPointerOperand(const IntrinsicInst *II) {
+    assert(false &&
+           "Target does not support querying of target memory intrinsics.");
     return nullptr;
   }
+
+  unsigned getTargetIntrinsicMatchingId(const IntrinsicInst *II) {
+    assert(false &&
+           "Target does not support querying of target memory intrinsics.");
+    return 0;
+  }
+
+  bool mayTargetIntrinsicReadFromMemory(const IntrinsicInst *II) {
+    assert(false &&
+           "Target does not support querying of target memory intrinsics.");
+    return true;
+  }
 };
 
 /// \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;

include/llvm/IR/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 {
+  // 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;
+
+  void setInstruction(ValTy *V) {
+    if (NativeInstr *Inst = dyn_cast<NativeInstr>(V)) {
+      I.setPointer(Inst);
+      I.setInt(true);
+    } else if (IntrinsicInstTy *II = dyn_cast<IntrinsicInstTy>(V)) {
+      if (Derived::isTargetIntrinsicSupported(II, this->TTI))
+        I.setPointer(II);
+        I.setInt(false);
+    }
+  }
+  bool isTargetIntrinsic() const {
+    assert(I.getPointer() && "Instruction not set");
+    return !I.getInt();
+  }
+
+public:
+  LoadStoreSiteBase(ValTy *V, const TargetTransformInfo &TTI)
+      : I(nullptr, false), TTI(&TTI) {
+    if (V != nullptr)
+      setInstruction(V);
+  }
+
+  /// isSimple - true if the load/store instruction or intrinsic is not volatile
+  /// or atomic.
+  bool isSimple() const {
+    InstrTy *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.
+  const Value *getPointerOperand() const {
+    InstrTy *Inst = I.getPointer();
+    if (isTargetIntrinsic())
+      return TTI->getTargetIntrinsicPointerOperand(cast<IntrinsicInstTy>(Inst));
+    return cast<NativeInstr>(Inst)->getPointerOperand();
+  }
+
+  LoadStoreSiteBase &operator=(InstrTy *Inst) {
+    I.setPointer(nullptr);
+    I.setInt(false);
+
+    if (Inst == nullptr)
+      return *this;
+
+    setInstruction(Inst);
+    return *this;
+  }
+
+  explicit operator bool() const { return I.getPointer() != nullptr; }
+  InstrTy *operator->() const { return I.getPointer(); }
+  InstrTy *getInstruction() const { return I.getPointer(); }
+  bool mayReadFromMemory() const {
+    InstrTy *Inst = I.getPointer();
+    if (isTargetIntrinsic())
+      return TTI->mayTargetIntrinsicReadFromMemory(cast<IntrinsicInst>(Inst));
+    return Inst->mayReadFromMemory();
+  }
+
+  /// 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 {
+    if (!isTargetIntrinsic()) {
+      if (const LoadInst *LI = dyn_cast<LoadInst>(Other))
+        return getPointerOperand()->getType() ==
+               LI->getPointerOperand()->getType();
+      else if (const StoreInst *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;
+  }
+
+  /// MatchInstructionAndLocation - true if Other is a compatible load or store
+  /// instruction, reading
+  /// from, or writing to the same memory location.
+  bool MatchInstructionAndLocation(const LoadStoreSiteBase &Other) const {
+    return getPointerOperand() == Other.getPointerOperand() &&
+           isCompatible(Other.getInstruction());
+  }
+};
+
+class LoadSite : public LoadStoreSiteBase<LoadSite, LoadInst> {
+  typedef LoadStoreSiteBase<LoadSite, LoadInst> Base;
+  friend class LoadStoreSiteBase;
+  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> {
+  typedef LoadStoreSiteBase<StoreSite, StoreInst> Base;
+  friend class LoadStoreSiteBase;
+  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

lib/Analysis/TargetTransformInfo.cpp

unsigned TargetTransformInfo::getReductionCost(unsigned Opcode, Type *Ty,
   return TTIImpl->getReductionCost(Opcode, Ty, IsPairwiseForm);
 }
 
 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::isTargetIntrinsicLikeLoad(
+    const IntrinsicInst *II) const {
+  return TTIImpl->isTargetIntrinsicLikeLoad(II);
 }
 
-Value *TargetTransformInfo::getOrCreateResultFromMemIntrinsic(
-    IntrinsicInst *Inst, Type *ExpectedType) const {
-  return TTIImpl->getOrCreateResultFromMemIntrinsic(Inst, ExpectedType);
+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, const 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(Function &)> TTICallback)

lib/Target/AArch64/AArch64TargetTransformInfo.h

public:
 
   unsigned getMemoryOpCost(unsigned Opcode, Type *Src, unsigned Alignment,
                            unsigned AddressSpace);
 
   unsigned getCostOfKeepingLiveOverCall(ArrayRef<Type *> Tys);
 
   void getUnrollingPreferences(Loop *L, TTI::UnrollingPreferences &UP);
 
-  Value *getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
-                                           Type *ExpectedType);
-
-  bool getTgtMemIntrinsic(IntrinsicInst *Inst, MemIntrinsicInfo &Info);
-
+  Value *getTargetIntrinsicPointerOperand(const IntrinsicInst *II);
+  bool isTargetIntrinsicLikeLoad(const IntrinsicInst *II);
+  bool isTargetIntrinsicLikeStore(const IntrinsicInst *II);
+  bool isTargetIntrinsicVolatile(const IntrinsicInst *II);
+  bool isTargetIntrinsicAtomic(const IntrinsicInst *II);
+  Value *getOrCreateResultFromTargetIntrinsic(IntrinsicInst *Inst,
+                                                    const Type *ExpectedType);
+  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 atomic.
+bool AArch64TTIImpl::isTargetIntrinsicAtomic(const IntrinsicInst *Inst) {
+  return false;
+}
+
+// AArch64 load/store intrinsics are not volatile.
+bool AArch64TTIImpl::isTargetIntrinsicVolatile(const IntrinsicInst *Inst) {
+  return false;
+}
+
+Value *
+AArch64TTIImpl::getOrCreateResultFromTargetIntrinsic(IntrinsicInst *Inst,
+                                                     const Type *ExpectedType) {
   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);
+    const 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);
+    Value *Res = UndefValue::get(const_cast<Type *>(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 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;
+  }
+
+bool AArch64TTIImpl::mayTargetIntrinsicReadFromMemory(
+    const IntrinsicInst *Inst) {
+  if (isTargetIntrinsicLikeLoad(Inst))
     return true;
+  assert(isTargetIntrinsicLikeStore(Inst));
+  return false;
 }

lib/Transforms/Scalar/EarlyCSE.cpp

 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/InstructionSimplify.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/LoadStoreSite.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/RecyclingAllocator.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include <deque>
public:
   /// 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.
   typedef RecyclingAllocator<
       BumpPtrAllocator,
-      ScopedHashTableVal<Value *, std::pair<Value *, unsigned>>>
+      ScopedHashTableVal<const Value *, std::pair<Value *, unsigned>>>
       LoadMapAllocator;
-  typedef ScopedHashTable<Value *, std::pair<Value *, unsigned>,
+  typedef ScopedHashTable<const 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;
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() { return Load; }
-    bool isStore() { return Store; }
-    bool isVolatile() { return Vol; }
-    bool isMatchingMemLoc(const ParseMemoryInst &Inst) {
-      return Ptr == Inst.Ptr && MatchingId == Inst.MatchingId;
-    }
-    bool isValid() { return Ptr != nullptr; }
-    int getMatchingId() { return MatchingId; }
-    Value *getPtr() { return Ptr; }
-    bool mayReadFromMemory() { return MayReadFromMemory; }
-    bool mayWriteToMemory() { 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
   // of the domtree node and all of the live out memory values are still current
   // in this block.  If this block has multiple predecessors, then they could
   // have invalidated the live-out memory values of our parent value.  For now,
   // just be conservative and invalidate memory if this block has multiple
   // predecessors.
   if (!BB->getSinglePredecessor())
     ++CurrentGeneration;
 
   /// 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++;
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()) {
+    if (LoadSite LS = LoadSite(Inst, TTI)) {
       // Ignore volatile loads.
-      if (MemInst.isVolatile()) {
+      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.
       std::pair<Value *, unsigned> InVal =
-          AvailableLoads.lookup(MemInst.getPtr());
-      if (InVal.first != nullptr && InVal.second == CurrentGeneration) {
+          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.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(), std::pair<Value *, unsigned>(
-                                                  Inst, CurrentGeneration));
+      AvailableLoads.insert(
+          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()))
+    StoreSite SS(Inst, TTI);
+    if (Inst->mayReadFromMemory() && !(SS && !SS.mayReadFromMemory()))
       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) {
for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E;) {
     }
 
     // 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 (SS) {
         // 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 (LastStore.MatchInstructionAndLocation(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(), std::pair<Value *, unsigned>(
-                                                    Inst, CurrentGeneration));
+        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;
 }