[PATCH] D58758: GlobalISel: Fix RegBankSelect for REG_SEQUENCE

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

> All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually.

Could you elaborate on that?

Essentially the logic here is: if at least one argument is set, we can use that reg bank for the destination, since this is just a glorified copy. I can see how this doesn't work for reg_sequence, but I don't get why phis are a problem.



================
Comment at: lib/CodeGen/GlobalISel/RegisterBankInfo.cpp:214
     if (IsCopyLike) {
-      OperandsMapping[0] = ValMapping;
-      CompleteMapping = true;
+      if (!OperandsMapping[0]) {
+        if (MI.isRegSequence()) {
----------------
arsenm wrote:
> qcolombet wrote:
> > Is it possible that `OperandsMapping[0] != nullptr` at that point?
> > 
> > Essentially when we set `OperandsMapping[0]` we have either `IsCopyLike == true` and we will `break` out right after we set it or we set it outside of `IsCopyLike == true` then we will never get into that block.
> It's initialized to null by the SmallVector constructor, so this is the first time it's ever set. The IsCopyLike check is a loop invariant.
> 
> All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually. I'm able to hack around reg_sequence in the backend now, but phi will require changing this and the iteration order to defer mapping phis until the operands are all already mapped
Ok, where I was getting at is, this check is not necessary, since by construction OperandsMapping[0] is nullptr and after that we won't loop back here.


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

https://reviews.llvm.org/D58758