This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Combine (logic_op (op x...), (op y...)) -> (op (logic_op x, y))
ClosedPublic

Authored by paquette on Jul 31 2020, 3:04 PM.

Details

Summary

This implements

(logic_op (op x...), (op y...)) -> (op (logic_op x, y))

when op is an extend, a shift, or an and.

This is similar to DAGCombiner::hoistLogicOpWithSameOpcodeHands (with a bunch of missing cases, e.g. G_TRUNC, G_BITCAST, etc.)

This is implemented so it works both pre and post-legalization.

I'm not entirely happy with the idea of creating (but not inserting) instructions in the match step, but it seems cleaner than passing around a bunch of registers etc. No strong opinions either way.

Diff Detail

Event Timeline

paquette created this revision.Jul 31 2020, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2020, 3:04 PM
paquette requested review of this revision.Jul 31 2020, 3:04 PM
aemerson added inline comments.Aug 4 2020, 11:47 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1834

I wonder if we can do this in a more functional style, and instead of storing the actual MachineInstr in the match info, store a function/lambda that can build and insert the instructions using a given MachineIRBuilder argument. You would have to modify the legality check here to use the other API call instead of passing an MachineInstr.

paquette added inline comments.Aug 4 2020, 11:51 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1834

Yeah, I was thinking of yanking the code from ComplexRendererFns and doing it that way. Wasn't sure about the best way to do it, but I can give it another shot.

arsenm added inline comments.Aug 4 2020, 11:59 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1795

Shouldn't construct a single use MachineIRBuilder. There should be one in CombinerHelper already

1824

Why not use the form with all operands in the initial construction?

1846

I don't think vectors are a problem?

paquette updated this revision to Diff 284493.Aug 10 2020, 1:33 PM
  • Use a more functional style + generalize a bit more so this can be used for other combines
  • Allow vectors + update test
  • Add a helper function for legality checks

Tests missing?

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
60

= default?

69

same.

paquette updated this revision to Diff 284800.Aug 11 2020, 9:54 AM
  • Add back tests
  • Use = default for default constructors
  • Add some asserts to applyBuildInstructionSteps
aemerson accepted this revision.Aug 11 2020, 10:20 AM

LGTM, thanks.

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