This is an archive of the discontinued LLVM Phabricator instance.

Handle big index in getelementptr instruction for AArch64FastISel
Needs ReviewPublic

Authored by chfast on Apr 21 2015, 1:30 AM.

Details

Reviewers
t.p.northover
Summary

This is a fix http://reviews.llvm.org/D8219 applied also to AArch64FastISel, as it has a copy of the code fixed previously.

Diff Detail

Event Timeline

chfast updated this revision to Diff 24100.Apr 21 2015, 1:30 AM
chfast retitled this revision from to Handle big index in getelementptr instruction for AArch64FastISel.
chfast updated this object.
chfast edited the test plan for this revision. (Show Details)
chfast added a reviewer: t.p.northover.
chfast added subscribers: chilledheart, Unknown Object (MLST).
t.p.northover edited edge metadata.Apr 21 2015, 9:42 AM

The code looks reasonable, but that test isn't going to work. It'll fail if those other backends aren't compiled in (lit.local.cfg explicitly requires only X86 in the test/CodeGen/X86 directory). Your best bet is probably to copy it into each target's subdirectory.

Cheers.

Tim.

chfast updated this revision to Diff 24151.Apr 21 2015, 12:31 PM
chfast edited the test plan for this revision. (Show Details)
chfast edited edge metadata.
chfast set the repository for this revision to rL LLVM.

Thank you for doing this fix, chfast.

I think we can probably copy this test into the directory of each target. Since the target R600 is still falling, we can add the part for R600 separately in a different patch.

Hi chfast and Northover:

It seems no one wants to fix the R600 backend part in a short time, so I filed the bug with R600 backend in the bugzilla for further reference. The bug's link is http://llvm.org/PR23312 .

Cheers,
Chilledheart

Thank you for doing this fix, chfast.

I think we can probably copy this test into the directory of each target. Since the target R600 is still falling, we can add the part for R600 separately in a different patch.

I'm not convinced about adding the test for every target. I added for arm64 as it was failing and fix is for that target. I don't know any other target beside R600 that fails on that.

Thank you for doing this fix, chfast.

I think we can probably copy this test into the directory of each target. Since the target R600 is still falling, we can add the part for R600 separately in a different patch.

I'm not convinced about adding the test for every target. I added for arm64 as it was failing and fix is for that target. I don't know any other target beside R600 that fails on that.

Or we can add this test for other targets later.

Do we have a consensus about the tests for other targets?

I think it's reasonable with just the AArch64 test added.

I'm not a massive fan of the way the test is written in general, but it might actually allow it to live in test/Generic once R600 is fixed.

Tim.

Are we doing something with that?