[PATCH] D132483: [AMDGPU][GlobalISel] Improve BFI Pattern Matching

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added a comment.

In D132483#3745119 <https://reviews.llvm.org/D132483#3745119>, @foad wrote:

> My gut feeling is that this is the wrong approach, because it will convert SALU to VALU operations regardless of whether they can actually be matched by some larger VALU pattern. This is bad for all sorts of reasons especially on GFX10+: latency, occupancy, power consumption.
>
> Instead I think the pattern matching for instructions like BFI needs to be able to look through cross-regbank copies. There has been some discussion in the past about this. Please read through https://discourse.llvm.org/t/globalisel-cross-bank-constant-propagation/57927 for a start.

I agree, this approach is very hacky and I started noticing its limits/drawbacks while I was trying to fix the unintended consequences of the new combine.

Do you think it'd be worthwhile to restart a new discussion on the Discourse? It seems like there's a lot of different opinions on how to solve this.
I'm personally not a fan of adding something like `g_maybe_cross_reg_bank_copy` - it feels like we'll need it everywhere sooner or later and it's making TableGen even more confusing.
Ignoring copies all the time also seems wrong because it'll just move the problem somewhere else. Later someone might want to match copies explicitly and then we'll end up with the same discussion.

Recently I've also had a lot of issues like this, where the DAG pattern is straightforward but the MIR one has extra additions making it harder to match.
I feel like there's an opportunity to improve pattern matching for those cases. On top of my head, maybe we could add a way to create special complex "PatFrags" for GISel that:

- In DAGISel, Map to one or more nodes (like current PatFrags)
- In GISel, run a C++ function to perform the matching

Then the C++ function could check the instruction & look through copies as needed. It'd have an advantage over ComplexPatterns as it could be used as a non-leaf node and won't require having an equivalent DAGISel function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132483