This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Honour ABI for rem under -O0 for EABI, GNUEABI, Android and Musl
ClosedPublic

Authored by rovka on Jul 1 2016, 4:38 AM.

Details

Summary

At higher optimization levels, we generate the libcall for DIVREM_Ix, which is
fine: aeabi_{u|i}divmod. At -O0 we generate the one for REM_Ix, which is the
default {u}mod{q|h|s|d}i3.

This commit makes sure that we don't generate REM_Ix calls for ABIs that
don't support them (i.e. where we need to use DIVREM_Ix instead). This is
achieved by bailing out of FastISel, which can't handle non-double multi-reg
returns, and letting the legalization infrastructure expand the REM_Ix calls.

It also updates the divmod-eabi.ll test to run under -O0 as well, and adds some
Windows checks to it to make sure we don't break things for it.

Fixes PR27068

Diff Detail

Event Timeline

rovka updated this revision to Diff 62480.Jul 1 2016, 4:38 AM
rovka retitled this revision from to [ARM] Honour ABI for rem under -O0 for EABI, GNUEABI, Android and Musl.
rovka updated this object.
rovka added a subscriber: llvm-commits.
rovka added a comment.Jul 1 2016, 4:42 AM

I'm not sure this is the most principled way to test this. I think unit tests for getLibcallName might be a better idea, but I notice we don't have an awful lot of unit tests. Any suggestions?

rengolin added inline comments.Jul 1 2016, 5:58 AM
test/CodeGen/ARM/divmod-eabi.ll
20

Removing the ABI registers will make the test a lot weaker and PR28164 tells me that this can already be broken in some cases.

Can you expand on what happens to the registers and why the test fails? My gut feeling is that a CHECK-DAG could fix the problem, but worst case scenario, we'll need two sets of CHECKs for O0 and O1.

rengolin updated this object.Jul 1 2016, 5:59 AM
rengolin updated this object.
rengolin updated this object.
rovka added a comment.Jul 1 2016, 6:05 AM

I'm not sure this is the most principled way to test this. I think unit tests for getLibcallName might be a better idea, but I notice we don't have an awful lot of unit tests. Any suggestions?

test/CodeGen/ARM/divmod-eabi.ll
20

I think it's just some spills getting in the way, I'll try CHECK-DAG. Thanks.

rovka updated this revision to Diff 62495.Jul 1 2016, 8:35 AM

Changed the test to check the registers too.

For the first 2 run lines (eabi, eabihf) i got away by adding -optimize-regalloc, but for the other lines I wasn't so lucky. It seems androideabi, gnueabi and musleabi all fail to merge div and divmod into a single call at O0, so I had to add another set of checks for that. I also had to add another set of checks for Darwin, because the scheduling is a bit different at O0 so instead of an add right after one of the libcalls we get a mov and then the corresponding add at the end of the function (I might be able to fiddle with some llc scheduling options to get rid of this...).

I think there's a problem with the androideabi, gnueabi and musleabi tests, which seem to be using r0 instead of r1 to get the remainder from idivmod (either that or it's just Friday evening and I'm being clumsy).

Please let me know if this looks like a good approach for the test and I'll update the rest of it (currently only the first function in the test is updated for the new sets of checks).

compnerd added inline comments.Jul 1 2016, 8:56 AM
test/CodeGen/ARM/divmod-eabi.ll
7

I think naming the prefix something more pertinent to what is being tested is helpful. How about EABI-RAOPT ?

16

Please add a test for non-EABI targets as do not get the EABI library calls.. How about something like thumbv7-windows?

43

Am I mistaken or couldnt these cases be collapsed? The idea is that on darwin targets, the calls are identical across the two? The math would be different, but you could have a single check for the calls in both cases.

rovka added a comment.Jul 1 2016, 1:56 PM

This patch is wrong, I need to work on it some more. Thanks for the comments, I'll incorporate them into the next version of the patch.

test/CodeGen/ARM/divmod-eabi.ll
48

+ ; DARWIN-O0: __modsi3

rovka updated this revision to Diff 62689.Jul 4 2016, 9:15 AM
rovka updated this object.
rovka added a subscriber: mcrosier.
  • Collapsed common checks between O0 and default
  • Added Windows checks (would be great if someone could double-check these, the Microsoft docs that I've found are pretty vague).
  • Fixed the patch to use the right register for the remainder. For this I had to basically bail out of FastISel, because it doesn't handle non-double multi-reg returns. I hope this hammer isn't too big for this problem, but getting FastISel to do the right thing here seemed like quite some work.
rengolin accepted this revision.Jul 8 2016, 9:02 AM
rengolin edited edge metadata.

Much simpler, great tests. Thanks! LGTM.

This revision is now accepted and ready to land.Jul 8 2016, 9:02 AM
This revision was automatically updated to reflect the committed changes.