This is an archive of the discontinued LLVM Phabricator instance.

[ARM64] Adds Cortex-A53 scheduling support for vector load/store post.
ClosedPublic

Authored by cestes on May 19 2014, 1:30 PM.

Details

Summary

Previously, the address output operand was excluded from the InstRWs
for the various vector load/store post increment instructions. This
patch corrects that.

Additionally, review revisions (D3769) were accidently omitted. They
weren't critical, so that patch stood and they're being added now.

Diff Detail

Event Timeline

cestes updated this revision to Diff 9584.May 19 2014, 1:30 PM
cestes retitled this revision from to [ARM64] Adds Cortex-A53 scheduling support for vector load/store post..
cestes updated this object.
cestes edited the test plan for this revision. (Show Details)
cestes added a subscriber: Unknown Object (MLST).May 19 2014, 2:02 PM
cestes added inline comments.May 19 2014, 2:42 PM
lib/Target/ARM64/ARM64SchedA53.td
221

This set of post-increment instructions still has regex with the _POST as optional.

mcrosier added inline comments.May 19 2014, 2:53 PM
lib/Target/ARM64/ARM64InstrInfo.cpp
883

I believe the usual convention is that the default case is at the top of the switch statement.

909

Same.

test/CodeGen/ARM64/misched-basic-A53.ll
113

Lines 114-116 aren't necessary. The git log should retain such history.

119

You could add a CHECK-LABEL directive.

; CHECK-LABEL: test_v16i8_post_imm_ld2

atrick accepted this revision.May 19 2014, 3:44 PM
atrick edited edge metadata.

LGTM

test/CodeGen/ARM64/misched-basic-A53.ll
113

I'm not sure what the official stance is now, but I like extra comments in test cases, referring to the source change and bug numbers (you can just say PR19761: title).

We avoid bug numbers in source code, but I think it's ok in test cases.

Codegen test cases can be very long-lived and difficult to maintain. It speeds things up a lot to explain what we're testing and why.

This revision is now accepted and ready to land.May 19 2014, 3:44 PM
mcrosier edited edge metadata.May 19 2014, 3:54 PM

Thanks, Andy. I'll push this shortly and update the PR.

LGTM

Comment at: test/CodeGen/ARM64/misched-basic-A53.ll:113
@@ +112,3 @@
+
+; Regression Test for Bug 19761
+; - [ARM64] Cortex-a53 schedule mode can't handle NEON post-increment

load

Chad Rosier wrote:

Lines 114-116 aren't necessary. The git log should retain such history.

I'm not sure what the official stance is now, but I like extra comments in
test cases, referring to the source change and bug numbers (you can just
say PR19761: title).

We avoid bug numbers in source code, but I think it's ok in test cases.

Codegen test cases can be very long-lived and difficult to maintain. It
speeds things up a lot to explain what we're testing and why.

http://reviews.llvm.org/D3829

mcrosier closed this revision.May 19 2014, 4:09 PM

Committed in r209176.