Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 3, 2024
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bdaeb78
[GISEL][AArch64] Stop using wip_match_opcode for some opcodes
madhur13490 Aug 30, 2024
58d02d4
Add MIR test
madhur13490 Aug 30, 2024
608c2cd
remove the test
madhur13490 Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions llvm/lib/Target/AArch64/AArch64Combine.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean there is no issue with removing wip_match_opcode?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is the issue.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[{ return matchICmpRedundantTrunc(*${root}, MRI, Helper.getKnownBits(), ${matchinfo}); }]),
(apply [{ applyICmpRedundantTrunc(*${root}, MRI, B, Observer, ${matchinfo}); }])>;

Expand Down Expand Up @@ -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); }])
>;
Expand Down Expand Up @@ -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}); }])
>;
Expand All @@ -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); }])
>;
Expand All @@ -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}); }])
>;
Expand Down
Loading