Page MenuHomePhabricator

[GlobalISel] Fix compiler crash lowering G_LOAD in AArch64.
ClosedPublic

Authored by drodriguez on Wed, Nov 27, 11:20 AM.

Details

Summary

When lowering G_LOAD in AArch64, a G_ANYEXT could have been inserted,
even if we were loading from a bigger stored and requesting a smaller
size.

The solution is changing the fixed usage of G_ANYEXT to a conditional
usage of TRUNC or G_ANYEXT depending on the size of the source and the
destination. This seems to avoid the crash in the legalizer.

A test case is included.

Diff Detail

Event Timeline

drodriguez created this revision.Wed, Nov 27, 11:20 AM

LGTM assuming @qcolombet or @aemerson have no objections.

Could you upload a diff with more context (-U9999)?

plotfi added a comment.Mon, Dec 2, 2:25 PM

Could you upload a diff with more context (-U9999)?

Agreed. More context would be good.

drodriguez updated this revision to Diff 231797.Mon, Dec 2, 4:03 PM

Provide more context. Rebase on top of master, just in case.

aemerson added inline comments.Mon, Dec 2, 5:04 PM
llvm/test/CodeGen/AArch64/GlobalISel/legalize-load-trunc.mir
15

This looks wrong. The resulting code is a load that generates an s10? But that value is then truncated down from s16 to s10? Shouldn't the load be 16 bit?

qcolombet added inline comments.Mon, Dec 2, 5:32 PM
llvm/test/CodeGen/AArch64/GlobalISel/legalize-load-trunc.mir
2

Could you generate the checks using the update_mir_test thing?
Like @aemerson pointed out the checks look wrong and I am wondering if you found another bug, or just forgot to update the test.

drodriguez marked 2 inline comments as done.Mon, Dec 2, 6:25 PM
drodriguez added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/legalize-load-trunc.mir
2

I didn’t know about the tool. I will generate the checks with it.

15

I think I forgot to run the FileCheck at the end to correct this things. I will send a new diff asap. Sorry for the confusion.

drodriguez updated this revision to Diff 231808.Mon, Dec 2, 6:27 PM

Generate the checks in the test with update_mir_test_checks.py. Check with FileCheck that the test actually passes.

aemerson accepted this revision.Tue, Dec 3, 1:39 PM

I think the test can be simplified a bit to not have the extra blocks and just testing the relevant instructions, otherwise LGTM.

This revision is now accepted and ready to land.Tue, Dec 3, 1:39 PM
drodriguez updated this revision to Diff 232024.Tue, Dec 3, 6:47 PM

Reduced the test case.

@aemerson: Thank you very much for accepting. I think I reduced the test case correctly. I needed to use the result of the G_LOAD, or the code was not being generated with the necessary G_TRUNC. I didn't imagine that it was as easy as returning the result. I hope the new test is better than the last.

Also, I don't have permission to merge, what's the next step for me to do? Thanks.

@aemerson: Thank you very much for accepting. I think I reduced the test case correctly. I needed to use the result of the G_LOAD, or the code was not being generated with the necessary G_TRUNC. I didn't imagine that it was as easy as returning the result. I hope the new test is better than the last.

Also, I don't have permission to merge, what's the next step for me to do? Thanks.

I can commit it for you later, after a few patches you can request commit access for yourself.

This revision was automatically updated to reflect the committed changes.