This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Enable mixed types in ARM CGP
ClosedPublic

Authored by samparker on Nov 5 2018, 8:38 AM.

Details

Summary

Previously, during the search, all values had to have the same 'TypeSize', which is equal to number of bits of the integer type of the icmp operand. All values in the tree had to match this size; meaning that, if we searched from i16, we wouldn't accept i8s. A change in type size requires zext and truncs to perform the casts so, to allow mixed narrow types, the handling of these instructions is now different:

  • zexts are sinks if the result is larger than TypeSize.
  • truncs are sinks if their result is less or equal to TypeSize.
  • truncs are sources if their incoming value is greater or equal than TypeSize.

Diff Detail

Repository
rL LLVM

Event Timeline

samparker created this revision.Nov 5 2018, 8:38 AM
SjoerdMeijer added inline comments.Nov 6 2018, 1:31 AM
lib/Target/ARM/ARMCodeGenPrepare.cpp
239 ↗(On Diff #172594)

I don't really understand the 'Equal' part in the GreaterOrEqual comparison. Can these types be equal, and is that what we want?

251 ↗(On Diff #172594)

By briefly looking at these new cases here and the tests, I think we are missing some tests.

samparker added inline comments.Nov 6 2018, 6:29 AM
lib/Target/ARM/ARMCodeGenPrepare.cpp
239 ↗(On Diff #172594)

Good question, looks like I've messed up there.

251 ↗(On Diff #172594)

Agreed.

samparker updated this revision to Diff 172762.Nov 6 2018, 6:58 AM

Now disallowing icmps that operate on types that are smaller than TypeSize. The handling of truncs being sources and sinks has also been reverted.

  • we allow casts if their result or operand is <= TypeSize.
  • zexts are sinks if their result > TypeSize.
  • truncs are still sinks if their operand == TypeSize.
  • truncs are still sources if their result == TypeSize.
SjoerdMeijer accepted this revision.Nov 9 2018, 1:16 AM

LGTM

test/CodeGen/ARM/CGP/arm-cgp-casts.ll
392 ↗(On Diff #172762)

nit: I think we should have 2 checks for uxtb here.

Same for the other tests below.

This revision is now accepted and ready to land.Nov 9 2018, 1:16 AM
This revision was automatically updated to reflect the committed changes.