[PATCH] D70564: [GlobalISel] LegalizationArtifactCombiner: Fix a bug in tryCombineMerges

[Volkan Keles via Phabricator via llvm-commits]

volkan marked an inline comment as done.
volkan added a comment.

In D70564#1757046 <https://reviews.llvm.org/D70564#1757046>, @volkan wrote:

> In D70564#1756698 <https://reviews.llvm.org/D70564#1756698>, @Petar.Avramovic wrote:
>
> > RegbankSelect doesn't track newly created instruction and I had to set reg bank manually.  Here is the patch that sets regbank for newly created COPY instructions F10828401: D70564_mips.diff <https://reviews.llvm.org/F10828401>.
>
>
> Thanks, I'll update the patch.


Actually, I think Mips changes should be a separate commit. Could you create a differential revision for it?



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:386
+        // Check if the constraints match. If so, replace the register.
+        // Otherwise, build a COPY instruction to avoid losing the constraints.
+        if (MRI.getRegBankOrNull(FromReg) == MRI.getRegBankOrNull(ToReg) &&
----------------
volkan wrote:
> Petar.Avramovic wrote:
> > aditya_nandakumar wrote:
> > > I think we're going to see this pattern multiple places. It would be unfortunate if we replicate this everywhere. I'd rather generate copies always and let the Combiner clean up redundant copies. Alternatively we could make a common function and make sure it's used everywhere (say replaceRegWithNewReg or something).
> > As far as I understand COPY should turn to cross bank move instruction. 
> > Considering all the places where LegalizationArtifactCombiner is used now I think that replaceRegWithNewRegOrCopy should do the following:
> > 
> >   - both FromReg and ToReg have different concrete bank/class: build copy (this is a cross regbank move).
> > 
> >   - FromReg has bank/class ToReg does not:  set ToReg's bank to Bank and `MRI.replaceRegWith(FromReg, ToReg)`. 
> >  If there is a regbank conflict here regBankSelect will find about it and make copy when it reaches  ToReg. There is  good chance we didn't need this copy so don't generate one. 
> > ```
> > %ToReg = G_ABC
> > %123 = G_MERGE %ToReg, ToReg1, ToReg2
> > %FromReg(Bank) , FromReg1, FromReg2 ... = G_UNMERGE %123
> > ... = G_XYZ %FromReg(Bank)
> > -> 
> > %ToReg(Bank) = G_ABC
> > ... = G_XYZ %ToReg(Bank) 
> > ```
> > 
> >   - ToReg has bank FromReg does not:  just `MRI.replaceRegWith(FromReg, ToReg)`.  ToReg will keep its bank. If we made copy here it would only have bank for use register and regbankselect would set same bank to def register (that copy is not cross regbank move).
> > ```
> > %ToReg(Bank) = G_ABC 
> > %123 = G_MERGE  %ToReg, ToReg1, ToReg2
> > %FromReg , FromReg1, FromReg2 ... = G_UNMERGE %123
> > ... = G_XYZ %FromReg
> > -> 
> > %ToReg(Bank) = G_ABC
> > ... = G_XYZ %ToReg(Bank) 
> > ```
> > 
> >   - otherwise FromReg and ToReg have same(or don't have one at all) class/bank: `MRI.replaceRegWith(FromReg, ToReg)` is fine.
> > 
> > In summary don't make copy if its likely to be redundant(i.e. not a cross register bank move). Also I we could use replaceRegWithNewRegOrCopy in all places where COPY is made during legalizer to reduce number or generated COPYs , e.g. MachineIRBuilder::buildAnyExtOrTrunc (used in trunc+anyext combine).
> > 
> I'll add a separate function for this and try to eliminate redundant COPY instructions.
Looks like we need to fix `CombinerHelper::matchCombineCopy` too. I'll create a differential revision for it first, then we share the same code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70564