This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Teach tryOptSelect to handle G_ICMP
ClosedPublic

Authored by paquette on Jul 2 2019, 9:36 AM.

Details

Summary

This teaches tryOptSelect to handle folding G_ICMP, and removes the requirement that the G_SELECT we're dealing with is floating point.

Some refactoring to make this work nicely as well:

  • Factor out the scalar case from the selection code for G_ICMP into emitIntegerCompare.
  • Make tryOptCMN return a MachineInstr* instead of a bool.
  • Make tryOptCMN not modify the instruction being selected.
  • Factor out the CMN emission into emitCMN for readability.

By doing this this way, we can get all of the compare selection optimizations in select emission.

Diff Detail

Repository
rL LLVM

Event Timeline

paquette created this revision.Jul 2 2019, 9:36 AM
aemerson accepted this revision.Jul 2 2019, 10:52 AM

Nice cleanup, some minor comments.

llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp
2777 ↗(On Diff #207579)

Can we just make these emit interfaces use Register instead? It doesn't seem like we need the full generality of MachineOperand.

3052 ↗(On Diff #207579)

And the same with Predicate too, we can just pass the actual CmpInst::Predicate value and avoid further casts,

This revision is now accepted and ready to land.Jul 2 2019, 10:52 AM
paquette marked an inline comment as done.Jul 2 2019, 11:10 AM
paquette added inline comments.
llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp
2777 ↗(On Diff #207579)

The only thing that makes that tricky, is that selectArithImmed takes a MachineOperand.

I guess that we could easily just compute it in emitIntegerCompare and pass the result along to this function though.

This revision was automatically updated to reflect the committed changes.
bjope added a subscriber: bjope.Jul 3 2019, 11:16 AM
bjope added inline comments.
llvm/trunk/lib/Target/AArch64/AArch64InstructionSelector.cpp
2816

This looks incorrect ( && has higher precedence than || ). So I assume it should be

assert((CmpTy.isScalar() || CmpTy.isPointer()) && "Expected scalar or pointer");

And we get a warning when building with gcc:

../lib/Target/AArch64/AArch64InstructionSelector.cpp:2938:48: error: suggest parentheses around '&&' within '||' [-Werror=parentheses]
assert(CmpTy.isScalar() || CmpTy.isPointer() && "Expected scalar or pointer");

paquette marked an inline comment as done.Jul 3 2019, 11:31 AM
paquette added inline comments.
llvm/trunk/lib/Target/AArch64/AArch64InstructionSelector.cpp
2816

Oh, you're right! Should be fixed in r365069.