This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Rewrite how VCMP are lowered, using a single node
ClosedPublic

Authored by dmgreen on Jul 22 2019, 4:56 AM.

Details

Summary

This removes the VCEQ/VCNE/VCGE/VCEQZ/etc nodes, just using two called VCMP and VCMPZ with an extra operand as the condition code. I believe this will make some combines simpler, allowing us to just look at these codes and not the operands. It also helps fill in a missing VCGTUZ MVE selection without adding extra nodes for it.

Diff Detail

Repository
rL LLVM

Event Timeline

dmgreen created this revision.Jul 22 2019, 4:56 AM
samparker added inline comments.Jul 22 2019, 6:55 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
11813 ↗(On Diff #211052)

There seem to be a load of different encodings for VCMP, do we have to think about that here at all when deciding whether a CC is valid?

11842 ↗(On Diff #211052)

This doesn't look like the correct formatting.

11843 ↗(On Diff #211052)

else if

11849 ↗(On Diff #211052)

else if

11862 ↗(On Diff #211052)

I know this isn't you, but these vectors should be SmallVectors instead.

dmgreen marked 6 inline comments as done.Jul 22 2019, 7:54 AM
dmgreen added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
11813 ↗(On Diff #211052)

The VCMPs are:
i: eq ne
s: gt, lt, ge, le
u: cs, hi
and the same set can be used for qq and qr compares.

Except in floating point, where there are only the first 6. The other 2 do not come up in PerformORCombine_i1 below, because we never have the opposite to invert them from. I've added a IsFloatingPoint argument for any other uses this function might get in the future.

11842 ↗(On Diff #211052)

Apparently this is. Too many long casts I guess, it splits it where it can.

11862 ↗(On Diff #211052)

Me in an earlier patch :) I'll change it there before committing.

dmgreen updated this revision to Diff 211101.Jul 22 2019, 7:57 AM
dmgreen marked 3 inline comments as done.
samparker accepted this revision.Jul 23 2019, 5:20 AM

Cheers! LGTM

This revision is now accepted and ready to land.Jul 23 2019, 5:20 AM
This revision was automatically updated to reflect the committed changes.