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

Repository
rL LLVM

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 ↗(On Diff #157198)

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

366 ↗(On Diff #157198)

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 ↗(On Diff #157970)

Nit: single statements aren't braced in llvm.

365 ↗(On Diff #157970)

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
1 ↗(On Diff #158711)

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.