[PATCH] D141747: Resolve a FIXME in MachineCopyPropagation by allowig propagation to subregister uses.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:604
+      SubregIdx = TRI->getSubRegIndex(CopyDstReg, MOUse.getReg());
+      assert(SubregIdx && "Unable to find index of subregister");
     }
----------------
resistor wrote:
> arsenm wrote:
> > This should bail as before rather than assert, the result class might not support the index
> Can you explain how that scenario could arise? I'm unable to picture it, and I didn't hit any cases of it in the test suite.
I don't know, but the API says it can fail and you can skip it as before. 


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:604
+      SubregIdx = TRI->getSubRegIndex(CopyDstReg, MOUse.getReg());
+      assert(SubregIdx && "Unable to find index of subregister");
     }
----------------
barannikov88 wrote:
> arsenm wrote:
> > resistor wrote:
> > > arsenm wrote:
> > > > This should bail as before rather than assert, the result class might not support the index
> > > Can you explain how that scenario could arise? I'm unable to picture it, and I didn't hit any cases of it in the test suite.
> > I don't know, but the API says it can fail and you can skip it as before. 
> I suppose it could be in the case of cross-class copy:
> ```
> $a = COPY $b
> use $sub_a
> ```
> Here, `$sub_a` is a sub-register of `$a` and its index is `sub_a_idx`.
> `$b` may not have `sub_a_idx` subregister index if `$a` and `$b` are in different register classes.
> 
I was also thinking of the cross bank case (but the API says it can fail, so it should be handled, or the unreachable needs to be moved into getSubRegIndex). I'd kind of expect the rest of allocation to try to avoid the cases where this would happen, but I assume a handcrafted MIR test could break this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141747