[PATCH] D145323: AMDGPU: Fix LiveVariables verifier error for values defined before SI_END_CF

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/lower-control-flow-live-variables-update.mir:248
+    %6:vgpr_32 = PHI %7, %bb.2, %1, %bb.0
+    %8:sreg_64 = S_MOV_B64 1
+    SI_END_CF killed %4, implicit-def $exec, implicit-def dead $scc, implicit $exec
----------------
arsenm wrote:
> ruiling wrote:
> > arsenm wrote:
> > > ruiling wrote:
> > > > arsenm wrote:
> > > > > ruiling wrote:
> > > > > > Can we teach the previous pass to insert the instruction after prologue instruction?  like through `SkipPHIsAndLabels()`.
> > > > > I think the prolog logic gets too complicated and we would need to scan through the entire block. Terminator placement is universally understood 
> > > > I don't see why we need to scan through the entire block?
> > > We currently treat the prolog as instructions that modify exec. The problem here is spills get inserted which don’t modify exec. In case a prolog appears anywhere, we would need to scan the whole block. The prolog concept is really frail as it is. We’d be better off removing it given that we sometimes need to insert more instructions in the prolog 
> > If an instruction modifies EXEC does not immediately follow PHI or other prolog instruction, I think it is not prolog anymore. So I think we should never look for prolog by scanning the whole block. I agree the prolog concept is fragile. But I don't have a good idea to replace it now. I like the other change to always split the block and let the s_or_exec be the terminator. With that change, do we still have a case to insert vector instructions before the s_or_exec afterward?
> > 
> > For this case, I would like we teach the constant materializer respect the basic block prolog unless we decided deprecating the prolog idea. Otherwise, such pattern can be easily broken by machine optimization passes (like sinking a vector operation before SI_END_CF).
> SI_END_CF is a hack to force placement of the instruction after phi lowering. It's fine if instruction end up inserted before it. In a better world where we have block arguments instead of phis, we wouldn't need to have this
IMHO, a vector instruction inserted before SI_END_CF is not ok. In a typical if/endif case, vector instructions in endif block should be executed with restored EXEC mask, which was done by SI_END_CF. I am not quite sure whether that would happen because vector instructions usually have dependency on EXEC, moving vector instruction across block should not be easy. Maybe I miss something, but I think it would be safer to keep SI_END_CF being prolog instruction.


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

https://reviews.llvm.org/D145323