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

[Tiehu Zhang via Phabricator via llvm-commits]

TiehuZhang added a comment.

In D107262#2922154 <https://reviews.llvm.org/D107262#2922154>, @dmgreen wrote:

> 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.

@dmgreen, thanks very much! We can make use of condition `UI->getParent() == TargetBB from if (UI->getParent() == TargetBB || isa<PHINode>(UI))` to update the insertPoint. Actually, the order of users obtained from `users()` may not follow instructions order in a basic block. So the previous patch was not correct.



================
Comment at: llvm/test/Transforms/CodeGenPrepare/AArch64/sink-free-instructions-1.ll:25
+  %broadcast.splatinsert143 = insertelement <4 x i32> poison, i32 %conv25, i32 0
+  br i1 undef, label %for.cond4.preheader.us.preheader, label %for.cond4.preheader.preheader
+
----------------
dmgreen wrote:
> Bugpoint has a habit of over-reducing test cases, introducing undef where they only make things worse and less maintainable in the longrun. Can you make the test not use any undef?
Thanks for you comment. The case is reduced from a csmith testcase,  so there are some strange instructions. I've made some changes to remove the `undef`.


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

https://reviews.llvm.org/D107262