[IR] Add getAccessType to Instruction

[luke]

Summary

There are multiple places in the code where the type of memory being accessed from an instruction needs to be obtained, including an upcoming patch to improve GEP cost modeling. This deduplicates the logic between them. It's not strictly NFC as EarlyCSE/LoopStrengthReduce may catch more intrinsics now.

[RKSimon]

Please can you add examples of its use (either in this patch or a follow child patch)?

[luke]

In D150583#4342696, @RKSimon wrote:

Please can you add examples of its use (either in this patch or a follow child patch)?

Hi there, didn't expect a review so quickly :) the child patch should be attached now.

The other places that I've seen use this are loopstrengthreduce and earlyCSE.

There's also the question as to whether or not this should subsume Type *getLoadStoreType(Value *I), also defined in Instructions.h.

[nikic]

Could you please also replace some reimplementations of this functionality with the helper, to show that it is generically useful?

llvm/lib/IR/Instruction.cpp

749

getPointerOperand()

753

This is incorrect for cmpxchg, which returns a struct.

[luke]

Fix AtomicCmpXChg type, use in EarlyCSE + LoopStrengthReduce

[luke]

llvm/lib/IR/Instruction.cpp

753

Thanks, good catch. I've plugged this into LoopStrengthReduce, which seems to have made the same mistake, so this patch is no longer NFC.

[nikic]

llvm/lib/IR/Instruction.cpp

749

Sorry, I meant to say getValueOperand() here...

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

900

Is this assertion right? After all we're also handling memcpy etc below.

[luke]

Address comments

[nikic]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

915

Is the drop of the MemTy assignment here intentional?

[luke]

Address review comments, remove unnecessary diff

[luke]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

915

Doesn't look like it. I've removed the unnecessary diffs, sorry about that.

[nikic]

LGTM

llvm/include/llvm/IR/Instruction.h

public:
   bool hasAtomicLoad() const LLVM_READONLY;
 
   /// Return true if this atomic instruction stores to memory.
   bool hasAtomicStore() const LLVM_READONLY;
 
   /// Return true if this instruction has a volatile memory access.
   bool isVolatile() const LLVM_READONLY;
 
+  /// Return the type this instruction accesses in memory, if any.
+  Type *getAccessType() const LLVM_READONLY;
+
   /// Return true if this instruction may throw an exception.
   ///
   /// If IncludePhaseOneUnwind is set, this will also include cases where
   /// phase one unwinding may unwind past this frame due to skipping of
   /// cleanup landingpads.
   bool mayThrow(bool IncludePhaseOneUnwind = false) const LLVM_READONLY;
 
   /// Return true if this instruction behaves like a memory fence: it can load

llvm/lib/IR/Instruction.cpp

if (auto *II = dyn_cast<IntrinsicInst>(this)) {
       case Intrinsic::matrix_column_major_store:
         return cast<ConstantInt>(II->getArgOperand(3))->isOne();
       }
     }
     return false;
   }
 }
 
+Type *Instruction::getAccessType() const {
+  switch (getOpcode()) {
+  case Instruction::Store:
+    return getPointerOperand(this)->getType();
+  case Instruction::Load:
+  case Instruction::AtomicRMW:
+    return getType();
+  case Instruction::AtomicCmpXchg:
+    return cast<AtomicCmpXchgInst>(this)->getNewValOperand()->getType();
+  case Instruction::Call:
+  case Instruction::Invoke:
+    if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(this)) {
+      switch (II->getIntrinsicID()) {
+      case Intrinsic::masked_load:
+      case Intrinsic::masked_gather:
+      case Intrinsic::masked_expandload:
+      case Intrinsic::vp_load:
+      case Intrinsic::vp_gather:
+      case Intrinsic::experimental_vp_strided_load:
+        return II->getType();
+      case Intrinsic::masked_store:
+      case Intrinsic::masked_scatter:
+      case Intrinsic::masked_compressstore:
+      case Intrinsic::vp_store:
+      case Intrinsic::vp_scatter:
+      case Intrinsic::experimental_vp_strided_store:
+        return II->getOperand(0)->getType();
+      default:
+        break;
+      }
+    }
+  }
+
+  return nullptr;
+}
+
 static bool canUnwindPastLandingPad(const LandingPadInst *LP,
                                     bool IncludePhaseOneUnwind) {
   // Because phase one unwinding skips cleanup landingpads, we effectively
   // unwind past this frame, and callers need to have valid unwind info.
   if (LP->isCleanup())
     return IncludePhaseOneUnwind;
 
   for (unsigned I = 0; I < LP->getNumClauses(); ++I) {

llvm/lib/Transforms/Scalar/EarlyCSE.cpp

public:
     Value *getPointerOperand() const {
       if (IntrID != 0)
         return Info.PtrVal;
       return getLoadStorePointerOperand(Inst);
     }
 
     Type *getValueType() const {
       // TODO: handle target-specific intrinsics.
-      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
-        switch (II->getIntrinsicID()) {
-        case Intrinsic::masked_load:
-          return II->getType();
-        case Intrinsic::masked_store:
-          return II->getArgOperand(0)->getType();
-        default:
-          return nullptr;
-        }
-      }
-      return getLoadStoreType(Inst);
+      return Inst->getAccessType();
     }
 
     bool mayReadFromMemory() const {
       if (IntrID != 0)
         return Info.ReadMem;
       return Inst->mayReadFromMemory();
     }
 

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

if (CmpX->getPointerOperand() == OperandVal)
       isAddress = true;
   }
   return isAddress;
 }
 
 /// Return the type of the memory being accessed.
 static MemAccessTy getAccessType(const TargetTransformInfo &TTI,
                                  Instruction *Inst, Value *OperandVal) {
-  MemAccessTy AccessTy(Inst->getType(), MemAccessTy::UnknownAddressSpace);
+  MemAccessTy AccessTy(Inst->getAccessType(), MemAccessTy::UnknownAddressSpace);
+  assert(AccessTy.MemTy);
   if (const StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
-    AccessTy.MemTy = SI->getOperand(0)->getType();
     AccessTy.AddrSpace = SI->getPointerAddressSpace();
   } else if (const LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
     AccessTy.AddrSpace = LI->getPointerAddressSpace();
   } else if (const AtomicRMWInst *RMW = dyn_cast<AtomicRMWInst>(Inst)) {
     AccessTy.AddrSpace = RMW->getPointerAddressSpace();
-  } else if (const AtomicCmpXchgInst *CmpX = dyn_cast<AtomicCmpXchgInst>(Inst)) {
+  } else if (const AtomicCmpXchgInst *CmpX =
+                 dyn_cast<AtomicCmpXchgInst>(Inst)) {
     AccessTy.AddrSpace = CmpX->getPointerAddressSpace();
   } else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
     switch (II->getIntrinsicID()) {
     case Intrinsic::prefetch:
     case Intrinsic::memset:
-      AccessTy.AddrSpace = II->getArgOperand(0)->getType()->getPointerAddressSpace();
-      AccessTy.MemTy = OperandVal->getType();
+      AccessTy.AddrSpace =
+          II->getArgOperand(0)->getType()->getPointerAddressSpace();
       break;
     case Intrinsic::memmove:
     case Intrinsic::memcpy:
       AccessTy.AddrSpace = OperandVal->getType()->getPointerAddressSpace();
       AccessTy.MemTy = OperandVal->getType();
       break;
     case Intrinsic::masked_load:
       AccessTy.AddrSpace =
           II->getArgOperand(0)->getType()->getPointerAddressSpace();
       break;
     case Intrinsic::masked_store:
-      AccessTy.MemTy = II->getOperand(0)->getType();
       AccessTy.AddrSpace =
           II->getArgOperand(1)->getType()->getPointerAddressSpace();
       break;
     default: {
       MemIntrinsicInfo IntrInfo;
       if (TTI.getTgtMemIntrinsic(II, IntrInfo) && IntrInfo.PtrVal) {
         AccessTy.AddrSpace
           = IntrInfo.PtrVal->getType()->getPointerAddressSpace();