[PATCH] D107262: [CodeGenPrepare] The instruction to be sunk should be inserted before its user in a block

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

In D107262#2921656 <https://reviews.llvm.org/D107262#2921656>, @TiehuZhang wrote:

> In D107262#2919594 <https://reviews.llvm.org/D107262#2919594>, @dmgreen wrote:
>
>> This happens because out of the chain of `insert; shuffle` only the `insert` needs to be sunk?
>> It may be better to remove the `UI->getParent() == TargetBB` from `if (UI->getParent() == TargetBB || isa<PHINode>(UI))` and update `InsertPoint` in the loop as we go. It won't need to clone the instruction, but it can update InsertPoint and continue.
>
> Hi, dmgreen, thanks for your review! But I don't quite make sense of your comment. The condition `UI->getParent() == TargetBB` is used to filter out some instructions already in targetBB to avoid the meaningless sinking, why can it be removed?

shouldSinkOperands will return a chain of instructions, in this case starting at the mul it will return the shuffle and the insert, as those are the instruction it is profitable to sink. They are visited in reverse order in order to sink the last instruction first. The shuffle is already in the TargetBB, so doesn't need to be sunk (or cloned), but we still need to update the InsertPoint when sinking the insert, or else it will fail dominance checks.

It looks like there are some Arm tests failing with the current attempt to fix that. The instruction being sunk may have multiple uses, and moving before the first one we happen to find might not always work, it might not be the instruction that was originally returned by shouldSinkOperands.

Instead, I think it would be better to make sure we are updating InsertPoint as we go in the ToReplace loop. So we add instruction to ToReplace even if they are already in TargetDB, and in the ToReplace loop we check if the instruction is already in the correct BB and just update the InsertPoint if so.



================
Comment at: llvm/test/Transforms/CodeGenPrepare/AArch64/sink-free-instructions-1.ll:7
+
+define void @c(i16 %e, i8 %f) {
+; CHECK-LABEL: @c(
----------------
Can this test be added to one of the existing files? There is already a sink-free-instructions.ll test. Otherwise the test should preferably have a better name than sink-free-instructions-1.ll.


================
Comment at: llvm/test/Transforms/CodeGenPrepare/AArch64/sink-free-instructions-1.ll:29
+  %broadcast.splat144 = shufflevector <4 x i32> %broadcast.splatinsert143, <4 x i32> poison, <4 x i32> zeroinitializer
+  %0 = mul <4 x i32> zeroinitializer, %broadcast.splat144
+  ret void
----------------
This value is dead? Can it be returned instead?


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

https://reviews.llvm.org/D107262