[PATCH] D150900: [InstCombine] Insert a bitcast to enable merging similar store insts

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1616
+    if (!SIVTy->isSingleValueType() || SIVTy->isVectorTy() ||
+        !OSVTy->isSingleValueType() || OSVTy->isVectorTy() ||
+        !CastInst::isBitOrNoopPointerCastable(OSVTy, SIVTy, DL) ||
----------------
These isSingleValueType / is VectorTy checks should not be necessary, isBitOrNoopPointerCastable will already do all the necessary type checks.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1689
+    OtherStore = NewOtherStore;
+  }
+
----------------
There is no need to rewrite to a new temporary store instruction. You can insert the bitcast directly at the phi node operand.

You also need to use the IRBuilder to create the cast, otherwise it will not be queued for reprocessing. In that case you also don't need the InsertBitcast flag, because the IRBuilder will omit the bitcast if it is not needed.

If you do everything correctly, then your AMDGPU problem should go away as well -- the instcombine-infinite-loop-threshold flag exists specifically to detect these worklist management bugs.


================
Comment at: llvm/test/Transforms/InstCombine/merging-multiple-stores-into-successor.ll:221
+
+define i16 @same_types_diff_align_no_merge(i1 %cond, i16 %a, i16 %b) {
+; CHECK-LABEL: @same_types_diff_align_no_merge(
----------------
arsenm wrote:
> Can't these merge using the minimum of the alignments?
It's possible, but please not in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150900