Page MenuHomePhabricator

[RFC][GISel] Add a way to ignore COPY instructions in InstructionSelector
AcceptedPublic

Authored by Pierre-vh on Oct 19 2022, 1:42 AM.

Details

Summary

RFC to add a way to ignore COPY instructions when pattern-matching MIR in GISel.

  • Add a new "GISelFlags" class to TableGen. Both Pattern and PatFrags defs can use it to alter matching behaviour.
  • Flags start at zero and are scoped: the setter returns a SaveAndRestore object so that when the current scope ends, the flags are restored to their previous values. This allows child patterns to modify the flags without affecting the parent pattern.
  • Child patterns always reuse the parent's pattern, but they can override its values. For more examples, see GlobalISelEmitterFlags.td tests.
  • [AMDGPU] Use the IgnoreCopies flag in BFI patterns, which are known to be bothered by cross-regbank copies.

Diff Detail

Event Timeline

Pierre-vh created this revision.Oct 19 2022, 1:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2022, 1:42 AM
Pierre-vh requested review of this revision.Oct 19 2022, 1:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2022, 1:42 AM
Pierre-vh planned changes to this revision.Nov 17 2022, 2:51 AM
Pierre-vh updated this revision to Diff 490794.Fri, Jan 20, 5:46 AM

Rebase, rework

Pierre-vh retitled this revision from [WIP] Allow ignoring copies in GISel to [GISel] Add a way to ignore COPY instructions in InstructionSelector.Fri, Jan 20, 5:47 AM
Pierre-vh edited the summary of this revision. (Show Details)
Pierre-vh retitled this revision from [GISel] Add a way to ignore COPY instructions in InstructionSelector to [WIP][GISel] Add a way to ignore COPY instructions in InstructionSelector.Fri, Jan 20, 5:47 AM
arsenm added inline comments.Fri, Jan 20, 5:56 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
5263

Would it be be possible to set this on a PatFrag level instead of a top-level pattern?

Pierre-vh retitled this revision from [WIP][GISel] Add a way to ignore COPY instructions in InstructionSelector to [WIP/RFC][GISel] Add a way to ignore COPY instructions in InstructionSelector.Mon, Jan 23, 12:17 AM
Pierre-vh edited the summary of this revision. (Show Details)
Pierre-vh planned changes to this revision.Mon, Jan 23, 12:24 AM
Pierre-vh updated this revision to Diff 491247.Mon, Jan 23, 1:02 AM
  • Add test

@arsenm for the PatFrags it looks like they don't (always) generate a Try block so the system as-is cannot handle them.
I was thinking of making GIM_SetFlag work only per-instruction and be used as a prefix, so instead of having

GIM_Try ...
  GIM_SetFlag GIMSF_IgnoreCopies

to set flags for a whole block we'd have

GIM_SetFlag, GIMSF_IgnoreCopies, GIM_Try ... # Sets the flag on the next instruction -> Try saves them in the whole block

Then we could have flags set for a single instruction which would allow us to support PatFrags and virtually anything else
after a bit more backend work:

GIM_SetFlag, GIMSF_IgnoreCopies, GIM_RecordInsn ...
Pierre-vh edited the summary of this revision. (Show Details)Mon, Jan 23, 1:03 AM
Pierre-vh planned changes to this revision.Wed, Jan 25, 2:51 AM

Current design doesn't work on PatFrags as they are inlined. I'm looking for solutions.

Pierre-vh updated this revision to Diff 492071.Wed, Jan 25, 5:06 AM

Rework so the system works on both Pattern and PatFrags

Pierre-vh retitled this revision from [WIP/RFC][GISel] Add a way to ignore COPY instructions in InstructionSelector to [RFC][GISel] Add a way to ignore COPY instructions in InstructionSelector.Wed, Jan 25, 5:07 AM
Pierre-vh edited the summary of this revision. (Show Details)
Pierre-vh updated this revision to Diff 493900.Wed, Feb 1, 3:56 AM
Pierre-vh marked an inline comment as done.

Rebase

This is ready for review, I don't have anymore changes to make for now.

I like the GISelFlags, looks pretty lightweight.
Are you planning to add more flags?
About IgnoreCopies, it looks correct to me. Considering the possibility to break constant bus limit, patch synergies well with GISelPredicateCode that checks for it.
Can you add look through copies to Three Op Pats from VOP3 td file?
Upcoming regbankselect changes should be able to deal with copies but nevertheless I would like to see this one merged.
LGTM but I would wait for feedback from other reviewers.

arsenm accepted this revision.Thu, Feb 2, 3:44 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/SIInstructions.td
2062

I think it's cleaner to hide these in an AMDGPUPat subclass, like AMDGPUPatIgnoreCopies

This revision is now accepted and ready to land.Thu, Feb 2, 3:44 AM
lkail added a subscriber: lkail.Thu, Feb 2, 5:19 AM
Pierre-vh updated this revision to Diff 495743.Wed, Feb 8, 12:11 AM
Pierre-vh marked an inline comment as done.

Address comment

Can this land (once the parent diff does) or does this need to wait for more people to approve?