[GISEL][AArch64][NFC] Stop using wip_match_opcode for some opcodes #106702

madhur13490 · 2024-08-30T10:18:02Z

This patch moves to the new style of writing
pattern for matching opcodes and thus deprecates using wip_match_opcoee.
It moves G_FCONSTANT, G_ICMP, G_STORE, and G_OR.

llvmbot · 2024-08-30T10:18:34Z

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-aarch64

Author: Madhur Amilkanthwar (madhur13490)

Changes

This match moves to the new style of writing
pattern for matching opcodes and thus deprecates using wip_match_opcoee.
It moves G_FCONSTANT, G_ICMP, G_STORE, and G_OR.

Full diff: https://github.com/llvm/llvm-project/pull/106702.diff

1 Files Affected:

(modified) llvm/lib/Target/AArch64/AArch64Combine.td (+8-8)

diff --git a/llvm/lib/Target/AArch64/AArch64Combine.td b/llvm/lib/Target/AArch64/AArch64Combine.td
index 3f717c8a60050f..d12f834da5a159 100644
--- a/llvm/lib/Target/AArch64/AArch64Combine.td
+++ b/llvm/lib/Target/AArch64/AArch64Combine.td
@@ -13,14 +13,14 @@ include "llvm/Target/GlobalISel/Combine.td"
 
 def fconstant_to_constant : GICombineRule<
   (defs root:$root),
-  (match (wip_match_opcode G_FCONSTANT):$root,
+  (match (G_FCONSTANT $dst, $src):$root,
          [{ return matchFConstantToConstant(*${root}, MRI); }]),
   (apply [{ applyFConstantToConstant(*${root}); }])>;
 
 def icmp_redundant_trunc_matchdata : GIDefMatchData<"Register">;
 def icmp_redundant_trunc : GICombineRule<
   (defs root:$root, icmp_redundant_trunc_matchdata:$matchinfo),
-  (match (wip_match_opcode G_ICMP):$root,
+  (match (G_ICMP $dst, $tst, $src1, $src2):$root,
          [{ return matchICmpRedundantTrunc(*${root}, MRI, Helper.getKnownBits(), ${matchinfo}); }]),
   (apply [{ applyICmpRedundantTrunc(*${root}, MRI, B, Observer, ${matchinfo}); }])>;
 
@@ -178,14 +178,14 @@ def adjust_icmp_imm_matchdata :
   GIDefMatchData<"std::pair<uint64_t, CmpInst::Predicate>">;
 def adjust_icmp_imm : GICombineRule <
   (defs root:$root, adjust_icmp_imm_matchdata:$matchinfo),
-  (match (wip_match_opcode G_ICMP):$root,
+  (match (G_ICMP $dst, $tst, $src1, $src2):$root,
           [{ return matchAdjustICmpImmAndPred(*${root}, MRI, ${matchinfo}); }]),
   (apply [{ applyAdjustICmpImmAndPred(*${root}, ${matchinfo}, B, Observer); }])
 >;
 
 def swap_icmp_operands : GICombineRule <
   (defs root:$root),
-  (match (wip_match_opcode G_ICMP):$root,
+  (match (G_ICMP $dst, $tst, $src1, $src2):$root,
           [{ return trySwapICmpOperands(*${root}, MRI); }]),
   (apply [{ applySwapICmpOperands(*${root}, Observer); }])
 >;
@@ -226,14 +226,14 @@ def build_vector_lowering : GICombineGroup<[build_vector_to_dup]>;
 
 def lower_vector_fcmp : GICombineRule<
   (defs root:$root),
-  (match (wip_match_opcode G_FCMP):$root,
+  (match (G_FCMP $dst, $tst, $src1, $src2):$root,
     [{ return matchLowerVectorFCMP(*${root}, MRI, B); }]),
   (apply [{ applyLowerVectorFCMP(*${root}, MRI, B); }])>;
 
 def form_truncstore_matchdata : GIDefMatchData<"Register">;
 def form_truncstore : GICombineRule<
   (defs root:$root, form_truncstore_matchdata:$matchinfo),
-  (match (wip_match_opcode G_STORE):$root,
+  (match (G_STORE $src, $addr):$root,
           [{ return matchFormTruncstore(*${root}, MRI, ${matchinfo}); }]),
   (apply [{ applyFormTruncstore(*${root}, MRI, B, Observer, ${matchinfo}); }])
 >;
@@ -254,7 +254,7 @@ def mutate_anyext_to_zext : GICombineRule<
 
 def split_store_zero_128 : GICombineRule<
   (defs root:$d),
-  (match (wip_match_opcode G_STORE):$d,
+  (match (G_STORE $src, $addr):$d,
           [{ return matchSplitStoreZero128(*${d}, MRI); }]),
   (apply [{ applySplitStoreZero128(*${d}, MRI, B, Observer); }])
 >;
@@ -277,7 +277,7 @@ def unmerge_ext_to_unmerge : GICombineRule<
 def regtriple_matchdata : GIDefMatchData<"std::tuple<Register, Register, Register>">;
 def or_to_bsp: GICombineRule <
   (defs root:$root, regtriple_matchdata:$matchinfo),
-  (match (wip_match_opcode G_OR):$root,
+  (match (G_OR $dst, $src1, $src2):$root,
          [{ return matchOrToBSP(*${root}, MRI, ${matchinfo}); }]),
   (apply [{ applyOrToBSP(*${root}, MRI, B, ${matchinfo}); }])
 >;

madhur13490 · 2024-08-30T10:20:34Z

It passed all existing tests in the trunk

tschuett · 2024-08-30T11:32:31Z

Independent of wip_match_opcode or not, I don't like these pattern because they match a single opcode, e.g., G_ICMP.
Aka it matches all G_ICMPs in the functions and not ,e.g., those used by a G_SELECT.

tschuett · 2024-08-30T11:36:03Z

llvm/lib/Target/AArch64/AArch64Combine.td

@@ -13,14 +13,14 @@ include "llvm/Target/GlobalISel/Combine.td"

 def fconstant_to_constant : GICombineRule<
  (defs root:$root),
-  (match (wip_match_opcode G_FCONSTANT):$root,
+  (match (G_FCONSTANT $dst, $src):$root,
         [{ return matchFConstantToConstant(*${root}, MRI); }]),
  (apply [{ applyFConstantToConstant(*${root}); }])>;

 def icmp_redundant_trunc_matchdata : GIDefMatchData<"Register">;


The name and the C++ code hint at a more precise pattern, which is not expressed in MIR.

Isn't $dst and $src just names given to operands? If so, what change would it bring in the matching algorithm?

Warning! Does not compile!

G_CONSTANT $rhs 0 G_TRUNC $lhs $input G_ICMP $root $pred $lhs $rhs

see

llvm-project/llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp

Line 92 in b0eefb4

if (!mi_match(LHS, MRI, m_GTrunc(m_Reg(WideReg))) ||

I added a MIR test called icmp-trunc.mir to test this scenario. The test compiles exactly the same before and after this commit. How can I modify the test to see the problem?

Your changes should NFC. You can put it into the PR title.

Do you mean there is no issue with removing wip_match_opcode?

tschuett · 2024-08-30T14:42:12Z

llvm/lib/Target/AArch64/AArch64Combine.td

         [{ return matchFConstantToConstant(*${root}, MRI); }]),
  (apply [{ applyFConstantToConstant(*${root}); }])>;

 def icmp_redundant_trunc_matchdata : GIDefMatchData<"Register">;
 def icmp_redundant_trunc : GICombineRule<
  (defs root:$root, icmp_redundant_trunc_matchdata:$matchinfo),
-  (match (wip_match_opcode G_ICMP):$root,
+  (match (G_ICMP $dst, $tst, $src1, $src2):$root,


This one is the issue.

All I am saying, it could reduce compile-time, if we replaced the G_ICMP with something ala:

G_CONSTANT $rhs 0 G_TRUNC $lhs $input G_ICMP $root $pred $lhs $rhs

The combiner would only jump into the C++ code, if there is a G_TRUNC on the lhs and a G_CONSTANT 0 on the rhs.
Currently it also would jump into the C++, if there would be, e.g., a G_UMULH on the lhs and immediately fail in the match and return false.

tschuett · 2024-08-31T05:31:40Z

https://llvm.org/docs/GlobalISel/MIRPatterns.html#gallery

dc03-work · 2024-09-02T04:28:06Z

llvm/test/CodeGen/AArch64/GlobalISel/icmp-trunc.mir

@@ -0,0 +1,19 @@
+


The RUN: line is missing, I think that is breaking the pre-commit CI.

Thanks but this test was added for the discussion with @tschuett and I don't intend to have it in the patch. This change is largely NFC and I am waiting for more reviews to land.

I would still suggest adding a RUN line even if you don't intend to include the test because it would make the CI non-red. As a reviewer, seeing green CI gives me more confidence about the patch.

I'd rather remove the test. Removed in the latest commit

Pierre-vh · 2024-09-03T13:45:00Z

I like this, if we can get rid of wip_match_opcode as a whole it'd be nice IMO

madhur13490 · 2024-09-03T14:05:20Z

I like this, if we can get rid of wip_match_opcode as a whole it'd be nice IMO

Thanks @Pierre-vh. Yes, I intend to try and move and probably have follow-up patches for the other opcodes.

This match moves to the new style of writing pattern for matching opcodes and thus deprecates using wip_match_opcoee. It moves G_FCONSTANT, G_ICMP, G_STORE, and G_OR.

llvmbot added the backend:AArch64 label Aug 30, 2024

madhur13490 requested review from davemgreen, aemerson, Pierre-vh and arsenm August 30, 2024 10:19

tschuett added the llvm:globalisel label Aug 30, 2024

tschuett reviewed Aug 30, 2024

View reviewed changes

madhur13490 force-pushed the madhur13490/remove_wip_match branch from b98cb61 to 0570943 Compare August 30, 2024 15:34

dc03-work reviewed Sep 2, 2024

View reviewed changes

madhur13490 changed the title ~~[GISEL][AArch64] Stop using wip_match_opcode for some opcodes~~ [GISEL][AArch64][NFC] Stop using wip_match_opcode for some opcodes Sep 2, 2024

madhur13490 added 3 commits September 3, 2024 19:47

[GISEL][AArch64] Stop using wip_match_opcode for some opcodes

bdaeb78

This match moves to the new style of writing pattern for matching opcodes and thus deprecates using wip_match_opcoee. It moves G_FCONSTANT, G_ICMP, G_STORE, and G_OR.

Add MIR test

58d02d4

remove the test

608c2cd

madhur13490 force-pushed the madhur13490/remove_wip_match branch from 5342a0f to 608c2cd Compare September 3, 2024 14:17

madhur13490 merged commit df159d3 into llvm:main Sep 3, 2024
4 of 6 checks passed

[GISEL][AArch64][NFC] Stop using wip_match_opcode for some opcodes #106702

[GISEL][AArch64][NFC] Stop using wip_match_opcode for some opcodes #106702

Uh oh!

Conversation

madhur13490 commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madhur13490 commented Aug 30, 2024

Uh oh!

tschuett commented Aug 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschuett commented Aug 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pierre-vh commented Sep 3, 2024

Uh oh!

madhur13490 commented Sep 3, 2024

Uh oh!

Uh oh!

Uh oh!

madhur13490 commented Aug 30, 2024 •

edited

Loading

llvmbot commented Aug 30, 2024 •

edited

Loading