This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Make multi-step legalization work.
ClosedPublic

Authored by kristof.beyls on Apr 26 2017, 4:00 AM.

Details

Summary

In r301116, a custom lowering needed to be introduced to be able to
legalize 8 and 16-bit divisions on ARM targets without a division
instruction, since 2-step legalization (WidenScalar from 8 bit to 32
bit, then Libcall the 32-bit division) doesn't work.

This fixes this and makes this kind of multi-step legalization, where
first the size of the type needs to be changed and then some action is
needed that doesn't require changing the size of the type,
straightforward to specify.

Diff Detail

Event Timeline

kristof.beyls created this revision.Apr 26 2017, 4:00 AM
rovka edited edge metadata.Apr 26 2017, 8:44 AM

Thanks for splitting this out! The ARM part looks excellent :)
I think you could add some target-independent tests for this in unittests/CodeGen/GlobalISel/LegalizerInfoTest.cpp, to make sure it works to WidenScalar/MoreElements etc to a type that has a Libcall or Custom action. That way we won't have to depend on ARM for testing this functionality.

Adding a unit test.

Thanks for splitting this out! The ARM part looks excellent :)
I think you could add some target-independent tests for this in unittests/CodeGen/GlobalISel/LegalizerInfoTest.cpp, to make sure it works to WidenScalar/MoreElements etc to a type that has a Libcall or Custom action. That way we won't have to depend on ARM for testing this functionality.

A yes, that makes sense. Now added.

Ping.

Thanks for the review on the ARM-specific part, Diana!
@t.p.northover : would you have time for a quick glance at the target-independent parts in LegalizerInfo.h, LegalizerInfo.cpp and LegalizerInfoTest.cpp?

Rebased to ToT & ping.

volkan added a subscriber: volkan.Jun 28 2017, 6:32 AM
t.p.northover accepted this revision.Jun 29 2017, 12:15 PM

Looks fine to me. Just one tiny nit (sorry).

include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
107

Functions should start with lower-case letters unless they need to match surrounding code.

This revision is now accepted and ready to land.Jun 29 2017, 12:15 PM
kristof.beyls marked an inline comment as done.Jun 30 2017, 1:25 AM
kristof.beyls added inline comments.
include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
107

D'oh! Of course.

Thanks for the review!

This revision was automatically updated to reflect the committed changes.