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

[Volkan Keles via Phabricator via llvm-commits]

volkan marked 2 inline comments as done.
volkan added a comment.

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.



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


================
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
+
----------------
Petar.Avramovic wrote:
> 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?
> What is the motivation for this in legalizer?
The patch doesn't do anything special in the legalizer. This tests is running legalizer because LegalizationArtifactCombiner is a part of the legalizer pass.

> To my understanding Legalizer should not be aware of register banks.
I don't think that's the case. It's possible to build target instructions with register constraints in (or before) the legalizer.

> Do you have some tests that run through the full globalisel pipeline and require this change to work?
I don't have any other tests as this is failing on our out-of-tree target, but I'm not sure why we need a test case running the whole pipeline.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70564