[llvm] [GISel] Funnel shift combiner port from SelectionDAG ISel to GlobalISel (PR #135132)
Axel Sorenson via llvm-commits
llvm-commits at lists.llvm.org
Tue May 6 04:33:28 PDT 2025
axelcool1234 wrote:
> I don't see why the hasOneUse checks are needed, and the DAG version that you copied does not have them.
I was told by @arsenm that it was a common combiner bug to lack the check. However, I do wonder:
@mshockwave You gave such a good explanation here https://github.com/llvm/llvm-project/pull/135132#discussion_r2051788654 about what's going on but I'm wondering if it's slightly incorrect now that we're applying a `GIReplaceReg`: In the combiner's current form, `$root` (which is the `OR` instruction) is replaced with the original `FSHR` instruction. If there are multiple uses of the `LSHR` instruction, would a `hasOneUse` check for that actually change anything? When `LSHR` has only one use, the replacement of the `OR` instruction leads to it having zero uses and not being emitted. If the `LSHR` has several uses, I suspect it'll be emitted as usual whether or not we have this `hasOneUse` check here since the combiner isn't removing it anymore (since it's simply replacing the `OR` instruction with the original `FSHR` instruction), right? Is my assessment of this correct?
I think my confusion stems from what happens when we do `(apply (G_FSHR $root, $z, $x, $y))` vs `(apply (GIReplaceReg $root, $out1))`. This is how I believe how it works:
- The first `apply` I mentioned removes all of the instructions that are matched with the resulting instruction in the `apply`
- The second `apply` I mentioned replaces the specified instruction
If that's correct, then I think @jayfoad might be right: if the `LSHR` instruction lacks other users, it'll never be emitted anyways (while it will if it does have users). Thus, we don't need the `hasOneUse` check.
https://github.com/llvm/llvm-project/pull/135132
More information about the llvm-commits
mailing list