This is an archive of the discontinued LLVM Phabricator instance.

Clang tests for ARM Cortex-A32 support.
ClosedPublic

Authored by samparker on Mar 21 2016, 4:27 AM.

Details

Reviewers
rengolin

Diff Detail

Event Timeline

samparker updated this revision to Diff 51145.Mar 21 2016, 4:27 AM
samparker retitled this revision from to Clang tests for ARM Cortex-A32 support..
samparker updated this object.
samparker added a subscriber: cfe-commits.
rengolin added inline comments.Mar 21 2016, 4:35 AM
test/Preprocessor/arm-target-features.c
349

The check-prefix here is misleading. Please change it.

samparker updated this revision to Diff 51165.Mar 21 2016, 8:08 AM

Changed A53-ARM and A53-THUMB test prefixes to ARMV8-ARM and ARMV8-THUMB.

rengolin accepted this revision.Mar 21 2016, 8:16 AM
rengolin added a reviewer: rengolin.

Hi Sam,

The patch looks good, thanks!

A few comments on the process:

  1. When you send an update, send the whole patch again, not just the new diff. Either squash or diff master, doesn't matter. If you don't have commit access, people will have to download the patch from this page to commit, and they'll need the whole change set.
  1. When you commit, don't commit the requested changes in separate. If the patch makes sense as one commit, it'll still need to be one commit after all code review changes. So, a git rebase squash will be necessary to bring the number of patches down to the original list before git svn dcommit.
  1. When you send a patch to review on Phab, please use "git show -U999", so that the patch is uploaded with context. I wish Phab was smart enough to know where the patch is being applied to and checkout the context from git/SVN, but apparently, it isn't. You can help by showing the context via -U option.

If you don't have commit rights, please upload the full patch to this review again, so I can commit it all at once.

cheers,
--renato

This revision is now accepted and ready to land.Mar 21 2016, 8:16 AM

Hi Renato,

Thanks for the tips, one patch makes much more sense. I'm currently in the process of obtaining commit access and this patch depends on http://reviews.llvm.org/D18239.

Thanks again,
sam

Right, if you could update both reviews with the full patch, I could commit them for you, for now.

samparker updated this revision to Diff 51173.Mar 21 2016, 9:03 AM
samparker edited edge metadata.

Combined the two diffs

rengolin closed this revision.Mar 21 2016, 10:35 AM

Committed in r263957.