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

Wed Mar 8 20:28:28 PST 2023

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

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

https://reviews.llvm.org/D145323