This is an archive of the discontinued LLVM Phabricator instance.

Differential D49778

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

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

Details

Reviewers
t.p.northover
echristo
javed.absar
efriedma
Commits
rG521dc4dda41e: Fix "Q" and "R" inline assembly template modifiers for big-endian Arm
rL341052: Fix "Q" and "R" inline assembly template modifiers for big-endian Arm
Summary

Consider the endianness of the target when printing register names. This is in line with the documentation at http://llvm.org/docs/LangRef.html#asm-template-argument-modifiers

Diff Detail

Repository
rL LLVM

Event Timeline

Jackson created this revision.Jul 25 2018, 12:06 AM
Herald added a reviewer: javed.absar. · View Herald TranscriptJul 25 2018, 12:06 AM
Herald added subscribers: chrib, kristof.beyls. · View Herald Transcript
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.

echristo added a comment.Jul 30 2018, 3:08 PM

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.

efriedma added a subscriber: efriedma.Jul 30 2018, 3:25 PM

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

llvm-commits added a subscriber: junbuml.Jul 30 2018, 3:26 PM

(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.

efriedma accepted this revision.Aug 29 2018, 11:10 AM

LGTM

This revision is now accepted and ready to land.Aug 29 2018, 11:10 AM
Jackson added a comment.Aug 30 2018, 1:13 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
In D49778#1218814, @Jackson wrote:

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

I can do that for you.

Closed by commit rL341052: Fix "Q" and "R" inline assembly template modifiers for big-endian Arm (authored by fhahn). · Explain WhyAug 30 2018, 3:30 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
Target/
ARM/
16 lines
test/
CodeGen/
ARM/
10 lines

Diff 163293

llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp

Show First 20 Lines • Show All 361 Lines • ▼ Show 20 Lines case 'Q': { // The least significant register of a pair.
// Later code expects OpNum to be pointing at the register rather than // Later code expects OpNum to be pointing at the register rather than
// the flags. // the flags.
OpNum += 1; OpNum += 1;
} }
unsigned NumVals = InlineAsm::getNumOperandRegisters(Flags); unsigned NumVals = InlineAsm::getNumOperandRegisters(Flags);
unsigned RC; unsigned RC;
bool FirstHalf;
const ARMBaseTargetMachine &ATM =
static_cast<const ARMBaseTargetMachine &>(TM);
// 'Q' should correspond to the low order register and 'R' to the high
// order register. Whether this corresponds to the upper or lower half
// depends on the endianess mode.
if (ExtraCode[0] == 'Q')
FirstHalf = ATM.isLittleEndian();
else
// ExtraCode[0] == 'R'.
FirstHalf = !ATM.isLittleEndian();
const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo(); const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
if (InlineAsm::hasRegClassConstraint(Flags, RC) && if (InlineAsm::hasRegClassConstraint(Flags, RC) &&
ARM::GPRPairRegClass.hasSubClassEq(TRI->getRegClass(RC))) { ARM::GPRPairRegClass.hasSubClassEq(TRI->getRegClass(RC))) {
if (NumVals != 1) if (NumVals != 1)
return true; return true;
const MachineOperand &MO = MI->getOperand(OpNum); const MachineOperand &MO = MI->getOperand(OpNum);
if (!MO.isReg()) if (!MO.isReg())
return true; return true;
const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo(); const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
unsigned Reg = TRI->getSubReg(MO.getReg(), ExtraCode[0] == 'Q' ? unsigned Reg = TRI->getSubReg(MO.getReg(), FirstHalf ?
ARM::gsub_0 : ARM::gsub_1); ARM::gsub_0 : ARM::gsub_1);
O << ARMInstPrinter::getRegisterName(Reg); O << ARMInstPrinter::getRegisterName(Reg);
return false; return false;
} }
if (NumVals != 2) if (NumVals != 2)
return true; return true;
unsigned RegOp = ExtraCode[0] == 'Q' ? OpNum : OpNum + 1; unsigned RegOp = FirstHalf ? OpNum : OpNum + 1;
if (RegOp >= MI->getNumOperands()) if (RegOp >= MI->getNumOperands())
return true; return true;
const MachineOperand &MO = MI->getOperand(RegOp); const MachineOperand &MO = MI->getOperand(RegOp);
if (!MO.isReg()) if (!MO.isReg())
return true; return true;
unsigned Reg = MO.getReg(); unsigned Reg = MO.getReg();
O << ARMInstPrinter::getRegisterName(Reg); O << ARMInstPrinter::getRegisterName(Reg);
return false; return false;
▲ Show 20 Lines • Show All 1,659 Lines • Show Last 20 Lines

llvm/trunk/test/CodeGen/ARM/print-registers.ll

; RUN: llc -mtriple=armeb-arm-none-eabi < %s -o -| FileCheck %s -check-prefixes=CHECK-BE
; RUN: llc -mtriple=arm-arm-none-eabi < %s -o -| FileCheck %s -check-prefixes=CHECK-LE
define dso_local void @_Z3fooi(i32 %a) local_unnamed_addr #0 {
entry:
; CHECK-BE: @ plain: [[LOW_REG:r[0-9]+]] Q: [[HIGH_REG:r[0-9]+]] R: [[LOW_REG]] H: [[HIGH_REG]]
; CHECK-LE: @ plain: [[LOW_REG:r[0-9]+]] Q: [[LOW_REG]] R: [[HIGH_REG:r[0-9]+]] H: [[HIGH_REG]]
tail call void asm sideeffect "// plain: $0 Q: ${0:Q} R: ${0:R} H: ${0:H}", "r"(i64 1) #1
ret void
}