This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Rewrite some simple rules using MIR Patterns
ClosedPublic

Authored by Pierre-vh on Aug 11 2023, 2:08 AM.

Details

Summary

Rewrites some simple rules that cause little to no codegen regressions as MIR patterns.

I may have missed some easy cases, but some other rules have intentionally been left as-is because bigger
changes are needed to make them work.

Diff Detail

Event Timeline

Pierre-vh created this revision.Aug 11 2023, 2:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 2:08 AM
Pierre-vh requested review of this revision.Aug 11 2023, 2:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 2:08 AM
foad added a comment.Aug 11 2023, 2:32 AM

Thanks! These are some really nice examples to study.

llvm/include/llvm/Target/GlobalISel/Combine.td
674–675

Is the order of instructions significant here? Would it still match if I wrote (match (G_FABS $dst, $tmp), (G_FNEG $tmp, $x)) instead?

llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir
12–14

What has caused this difference?

Pierre-vh marked an inline comment as done.Aug 11 2023, 3:27 AM
Pierre-vh added inline comments.
llvm/include/llvm/Target/GlobalISel/Combine.td
674–675

The order doesn't matter at all, it emits patterns by walking the "tree" from the root instruction, following the operands to their defs recursively
For instance below add_sub_reg_frags orders the patterns differently

llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir
12–14

Below in test_combine_select_undef_res0_res1 we have a copy $x0 instead of $x1, not sure why

Pierre-vh marked an inline comment as done.Aug 11 2023, 3:28 AM
Pierre-vh added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-not-really-equiv-insts.mir
53

It'd be nice if CSE allowed simple invariant loads, then this wouldn't be an issue, but I assume CSEing loads is not a trivial task?

foad added inline comments.Aug 11 2023, 3:37 AM
llvm/test/CodeGen/AArch64/GlobalISel/combine-select.mir
12–14

Oh I see, nothing has really changed in test_combine_select_same_res. Could you precommit a patch that just regenerates the checks for this file?

Pierre-vh updated this revision to Diff 549324.Aug 11 2023, 3:45 AM
Pierre-vh marked 2 inline comments as done.

Rebase on regenerated checks

foad added inline comments.Aug 11 2023, 3:56 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
364

This 2 was a bug - in G_SELECT $dst, $undef, $x, $y it was picking $x but the comment says "y".

arsenm accepted this revision.Aug 11 2023, 9:58 AM
This revision is now accepted and ready to land.Aug 11 2023, 9:58 AM
arsenm added inline comments.Aug 11 2023, 9:58 AM
llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-trivial-arith.mir
313–316

This regressed?

arsenm requested changes to this revision.Aug 11 2023, 9:58 AM
This revision now requires changes to proceed.Aug 11 2023, 9:58 AM
arsenm accepted this revision.Aug 11 2023, 10:00 AM
arsenm added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-trivial-arith.mir
313–316

I think I see why, this is the lack of constant folding extensions which I have a patch for. I think we need less look through handling and more consistent constant and copy folding

This revision is now accepted and ready to land.Aug 11 2023, 10:00 AM

Very nice to see the first concrete improvements!

This revision was landed with ongoing or failed builds.Aug 22 2023, 12:10 AM
This revision was automatically updated to reflect the committed changes.