Page MenuHomePhabricator

[GlobalISel] LegalizationArtifactCombiner: Fix a bug in tryCombineMerges
ClosedPublic

Authored by volkan on Nov 21 2019, 1:32 PM.

Details

Summary

Like COPY instructions explained in D70616, we don't check the constraints
when combining G_UNMERGE_VALUES. Use the same logic used in D70616 to check
if registers can be replaced, or a COPY instruction needs to be built.

Diff Detail

Event Timeline

volkan created this revision.Nov 21 2019, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2019, 1:32 PM

@Petar.Avramovic Could you take a look at the failing Mips tests?

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
431

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

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

.

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
431

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

llvm/test/CodeGen/AArch64/GlobalISel/artifact-combine-merge-with-constraints.mir
2 ↗(On Diff #230524)

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?

volkan marked 2 inline comments as done.Nov 22 2019, 10:04 AM

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

.

Thanks, I'll update the patch.

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
431

I'll add a separate function for this and try to eliminate redundant COPY instructions.

llvm/test/CodeGen/AArch64/GlobalISel/artifact-combine-merge-with-constraints.mir
2 ↗(On Diff #230524)

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.

volkan marked an inline comment as done.Nov 22 2019, 11:01 AM

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

.

Thanks, I'll update the patch.

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

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
431

Looks like we need to fix CombinerHelper::matchCombineCopy too. I'll create a differential revision for it first, then we share the same code.

volkan marked an inline comment as done.Nov 22 2019, 1:26 PM
volkan added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
431

Created D70616.

volkan updated this revision to Diff 230726.Nov 22 2019, 2:35 PM
volkan edited the summary of this revision. (Show Details)
  • Added a function called canReplaceReg to share the code.
  • Added a function called replaceRegOrBuildCopy that tries to replace the registers if the constraints match, and builds a COPY otherwise.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
289

We should probably be notifying the observer here...

volkan updated this revision to Diff 230734.Nov 22 2019, 3:16 PM
  • Notify the observer when replacing registers.
volkan marked 2 inline comments as done.Nov 22 2019, 3:17 PM
volkan added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
289

Right, updated the patch.

With removed xfail from tests, this looks good to me. I would like to hear if other reviewers have comments.

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
285

Where will we use replaceRegOrBuildCopy so it has to be static?
I think that eventually Observer should end up as field in LegalizationArtifactCombiner like in LegalizerHelper. There are a few LLVM_DEBUG(dbgs() << ".. Combine MI: " << MI;); around and Observer could handle them.

llvm/test/CodeGen/Mips/GlobalISel/regbankselect/long_ambiguous_chain_s32.mir
3–6 ↗(On Diff #230734)

With updated patch there are no changes in regbankselect tests, you can remove XFAIL. Register we are replacing in combine has no constraints.

volkan marked 2 inline comments as done.Dec 2 2019, 12:36 PM
volkan added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
285

I'm not sure if we need to have a separate function for this, I added it based on the initial review. I can move this logic into tryCombineMerges as an alternative. Regarding the observer, it makes sense to pass the observer to simplify the functions, but it probably shouldn't be a part of this change. Also, I'm not sure if we still want to keep LegalizationArtifactCombiner as discussed in D65894.

llvm/test/CodeGen/Mips/GlobalISel/regbankselect/long_ambiguous_chain_s32.mir
3–6 ↗(On Diff #230734)

I'll do that, thanks!

volkan updated this revision to Diff 231767.Dec 2 2019, 12:52 PM
  • Removed XFAILs.
paquette added inline comments.Jan 6 2020, 2:17 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
289

I think we can simplify the logic a bit here:

if (!llvm::canReplaceReg(DstReg, SrcReg, MRI) {
   Builder.buildCopy(DstReg, SrcReg);
   return;
}

SmallVector<MachineInstr *, 4> UseMIs;
// ...
llvm/lib/CodeGen/GlobalISel/Utils.cpp
165

Redundant comment?

172

Don't need to check both DstTy.isValid() and SrcTy.isValid(), since DstTy.isValid() == SrcTy.isValid() at this point.

volkan updated this revision to Diff 242703.Feb 5 2020, 11:06 AM
  • Rebased and updated based on the feedback.
volkan marked 3 inline comments as done.Feb 5 2020, 11:06 AM
arsenm added inline comments.Feb 5 2020, 11:25 AM
llvm/lib/CodeGen/GlobalISel/Utils.cpp
162

You can do .isPhysical()

171–172

This still looks redundant to me. (DstTy.isValid() != SrcTy.isValid()) is implied by DstTy != SrcTy

175–178

Just check getRegClassOrRegBank(Src) == getRegClassOrRegBank(Dst)?

183–185

Check these first and short circuit the source checks?

183–185

This can also use getRegClassOrRegBank instead of checking the two separately?

volkan updated this revision to Diff 242726.Feb 5 2020, 12:21 PM
  • Simplified llvm::canReplaceReg(..).
volkan marked 5 inline comments as done.Feb 5 2020, 12:21 PM
nhaehnle removed a subscriber: nhaehnle.Feb 7 2020, 3:18 AM
arsenm accepted this revision.Feb 13 2020, 2:57 PM

LGTK

This revision is now accepted and ready to land.Feb 13 2020, 2:57 PM
arsenm added inline comments.Feb 13 2020, 2:57 PM
llvm/lib/CodeGen/GlobalISel/Utils.cpp
168–171

You can fold this into return x || (...

volkan updated this revision to Diff 244699.Feb 14 2020, 10:07 AM
  • Simplified llvm::canReplaceReg(..) a bit more.
volkan marked an inline comment as done.Feb 14 2020, 10:08 AM