[PATCH] D87107: [AMDGPU] Target hook to apply target specific split constraint

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In D87107#2257000 <https://reviews.llvm.org/D87107#2257000>, @alex-t wrote:

> In D87107#2256913 <https://reviews.llvm.org/D87107#2256913>, @rampitec wrote:
>
>> In D87107#2256864 <https://reviews.llvm.org/D87107#2256864>, @alex-t wrote:
>>
>>>> Also I still think that disabling a whole "endif" block is an overkill.
>>>
>>> It only is disabled if S_OR_B64 exec, ... is in the middle of the block that should never happen.
>>>
>>>   while (isBasicBlockPrologue(*J)) {
>>>     if (IsExecRestore(&*J))
>>>       return true;
>>>
>>> assumes that if exec is restored in the block prologue it is valid
>>
>> So practically it never happens and split is effectively only disabled in an empty block? I said it already: it does not matter that exec is zero, what matters is that it does not match. It does not matter that a block is empty as well, it is enough to split before s_or to hit the bug.
>
> Let's forget about exec == 0...
> The code snippet above looks for exec restoring instruction in the block prologue and return true if it exists.  So, false positive can only happen if there is no exec restoring instruction in the beginning of the block but it is placed in the middle of the block. In this case I report the block is invalid for splitting despite it is valid to split after the exec restoring instruction in the middle of the block. Do we really have exec restored in the middle of the block?

No, we do not change exec in the middle of the block.
It would help to understand what are you doing if you would have a test to show.



================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:6817
+    MachineBasicBlock::iterator I = Pred->getFirstTerminator();
+    if (I != Pred->end() && I->modifiesRegister(AMDGPU::EXEC, TRI)) {
+      while (++I != Pred->end()) {
----------------
alex-t wrote:
> rampitec wrote:
> > S_OR shall not be a terminator, it is used in the prologue.
> This is not about searching S_OR at all. This code checks if one of the predecessors has exec modified at block exit and a branch with exeec == 0 to current block or which we are querying now.
It does at line 6811?


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

https://reviews.llvm.org/D87107