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

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added a comment.

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



================
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) &&
----------------
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).



================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/artifact-combine-merge-with-constraints.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -o - -march=aarch64 -run-pass=legalizer %s | FileCheck %s
+
----------------
What is the motivation for this in legalizer?
To my understanding Legalizer should not be aware of register banks. Do you have some tests that run through the full globalisel pipeline and require this change to work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70564