This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Only lower to interleaved load/store if the target has NEON
ClosedPublic

Authored by jketema on Oct 7 2015, 6:13 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jketema updated this revision to Diff 36731.Oct 7 2015, 6:13 AM
jketema retitled this revision from to [ARM] Only lower to interleaved load/store if the target has NEON.
jketema updated this object.
jketema added a reviewer: sbaranga.
jketema added a subscriber: llvm-commits.
sbaranga edited edge metadata.Oct 7 2015, 6:21 AM

Thanks, Jeroen! This makes sense to me. I think we also need a test for the interleaved store lowering (I see that there is only a test for loads).

Also, would we need a similar change for AArch64?

Thanks,
Silviu

test/CodeGen/ARM/arm-interleaved-accesses-bug.ll
1 ↗(On Diff #36731)

Is -mcpu=cortex-a9 required here?

Ouch! Thanks for the fix. :)

Could you add that run line to the existing test file and CHECK-NOTs to the respective places instead of in a new file?

@rengolin If it is possible to conditionalize the CHECK-NOT on the run line. Is that possible? I haven't seen that before.

test/CodeGen/ARM/arm-interleaved-accesses-bug.ll
1 ↗(On Diff #36731)

This was the easiest way for me to reproduce the crash. Just disabling NEON didn't do the trick. I'll have a look at exactly which attributes are needed to hit this particular code path.

sbaranga added inline comments.Oct 7 2015, 6:46 AM
test/CodeGen/ARM/arm-interleaved-accesses-bug.ll
1 ↗(On Diff #36731)

Strange. This still triggers for me after removing the -mcpu.

jketema added inline comments.Oct 7 2015, 6:48 AM
test/CodeGen/ARM/arm-interleaved-accesses-bug.ll
1 ↗(On Diff #36731)

I might have been using a different triple before. Dropping -mcpu also works for me. I'll drop it.

@rengolin If it is possible to conditionalize the CHECK-NOT on the run line. Is that possible? I haven't seen that before.

Yeah, you just add:

; RUN: llc -mtriple=armv7a-eabi -mattr=-neon < %s | FileCheck --check-prefix=NONEON %s

then

; CHECK-NONEON-NOT: vld2

etc.

jketema updated this revision to Diff 36741.Oct 7 2015, 7:34 AM
jketema edited edge metadata.

Also fix AArch64 and add the run line to existing test file instead (this also adds coverage for store tests).

rengolin accepted this revision.Oct 7 2015, 7:39 AM
rengolin added a reviewer: rengolin.

LGTM. Thanks!

This revision is now accepted and ready to land.Oct 7 2015, 7:39 AM
This revision was automatically updated to reflect the committed changes.

Committed. Thanks for the quick feedback!