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

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

Please add tests for volatile loads.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:561
+
+  // Visit every user of this load instruction,
+  for (User *U : LI.users()) {
----------------
This comment is pretty obvious, I'd rather not have it.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:563
+  for (User *U : LI.users()) {
+    // All users must be casts.
+    auto *CI = dyn_cast<CastInst>(U);
----------------
Same here.


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


================
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.
----------------
Does this bail on address space casts to different address spaces (when the value is a pointer)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75505