Page MenuHomePhabricator

Fix "Q" and "R" inline assembly template modifiers for big-endian Arm
ClosedPublic

Authored by Jackson on Jul 25 2018, 12:06 AM.

Diff Detail

Event Timeline

Jackson created this revision.Jul 25 2018, 12:06 AM
thopre added a subscriber: thopre.Jul 30 2018, 7:38 AM

Hi Jackson, can you reupload the patch with more context line? Something like git diff -U99999 (arc diff does it automatically). Thanks.

thopre added inline comments.Jul 30 2018, 7:48 AM
lib/Target/ARM/ARMAsmPrinter.cpp
365–370

Alternatively: FirstHalf = (ExtraCode[0] == 'Q') == ATM.isLittleEndian(), your call on whether this is clearer or not to read.

366

Indentation looks off here.

Jackson updated this revision to Diff 157970.Jul 30 2018, 8:32 AM

Thanks for the review Thomas. I have update the diff so it has context, and have fixed the formatting issue. I think the if statement is probably better as is, although I'm also not entirely sure.

Couple of comments.

lib/Target/ARM/ARMAsmPrinter.cpp
365

Nit: single statements aren't braced in llvm.

365

Block comment explaining why and how this affects things.

(I think this needs to be rebased on top of D49984.)

(Yep)

Jackson updated this revision to Diff 158711.Aug 2 2018, 3:01 AM
Jackson marked an inline comment as done.

Thanks for the reviews. I have rebased this patch, added a comment to explain what is going on, and removed the redundant curly braces.

Jackson marked 3 inline comments as done.Aug 20 2018, 6:14 AM

ping

efriedma added inline comments.Aug 21 2018, 11:45 AM
test/CodeGen/ARM/print-registers-be.ll
2

Could you add a RUN line for little-endian as well?

Jackson updated this revision to Diff 163081.Aug 29 2018, 7:15 AM

Added a testcase for little-endian.

This revision is now accepted and ready to land.Aug 29 2018, 11:10 AM

As I do not have commit rights, could this be committed on my behalf?

fhahn added a subscriber: fhahn.Aug 30 2018, 2:01 AM

As I do not have commit rights, could this be committed on my behalf?

I can do that for you.

This revision was automatically updated to reflect the committed changes.