[PATCH] D75505: [InstCombine] Enhance cast-of-load->load-of-bitcast fold to handle multiple identical casts

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:571
+      // Note that if load is atomic, we might not be able to change load type.
+      if (LI.isAtomic() && !isSupportedAtomicType(DestTy))
+        return nullptr;
----------------
jfb wrote:
> This seems like a pre-existing bug: you should bail on volatile as well. For example, if user code does a volatile load of int32 and then casts to float, we should probably try to emit an integer load (even if that's not guaranteed by the language).
I can do that, but could you point me to the appropriate langref wording?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:576
+    if (DestTy != CI->getDestTy())
+      return nullptr;
+    // The cast must actually be something that can be folded into load.
----------------
jfb wrote:
> Does this bail on address space casts to different address spaces (when the value is a pointer)?
```
/// This function determines if the CastInst does not require any bits to be
/// changed in order to effect the cast. Essentially, it identifies cases where
/// no code gen is necessary for the cast, hence the name no-op cast.  For
/// example, the following are all no-op casts:
/// # bitcast i32* %x to i8*
/// # bitcast <2 x i32> %x to <4 x i16>
/// # ptrtoint i32* %x to i32     ; on 32-bit plaforms only
/// Determine if the described cast is a no-op.
bool CastInst::isNoopCast(Instruction::CastOps Opcode,
                          Type *SrcTy,
                          Type *DestTy,
                          const DataLayout &DL) {
  switch (Opcode) {
    default: llvm_unreachable("Invalid CastOp");
    case Instruction::Trunc:
    case Instruction::ZExt:
    case Instruction::SExt:
    case Instruction::FPTrunc:
    case Instruction::FPExt:
    case Instruction::UIToFP:
    case Instruction::SIToFP:
    case Instruction::FPToUI:
    case Instruction::FPToSI:
    case Instruction::AddrSpaceCast:
      // TODO: Target informations may give a more accurate answer here.
      return false;
    case Instruction::BitCast:
      return true;  // BitCast never modifies bits.
    case Instruction::PtrToInt:
      return DL.getIntPtrType(SrcTy)->getScalarSizeInBits() ==
             DestTy->getScalarSizeInBits();
    case Instruction::IntToPtr:
      return DL.getIntPtrType(DestTy)->getScalarSizeInBits() ==
             SrcTy->getScalarSizeInBits();
  }
}
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:589
+  for (CastInst *CI : Casts) {
+    CI->replaceAllUsesWith(NewLoad);
+    IC.eraseInstFromFunction(*CI);
----------------
nikic wrote:
> This should use `replaceInstUsesWith()`.
I suppose it would be good to eradicate usages of "wrong" spellings so we don't accidentally use the "deprecated" one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75505/new/

https://reviews.llvm.org/D75505