[llvm-commits] Fixing Bug 13662: paired register for inline asm with 64-bit data on ARM

Weiming Zhao weimingz at codeaurora.org
Fri Nov 16 15:37:11 PST 2012 

Hi Jim,

 

Attached is the follw-up patch, which renames PairXRegs to
createXRegPairNode and QuadXRegs => createQuadXRegsNode to meet the coding
style requirement. 

 

There are also tons of SelectXYZ() functions that should be selectXYZ().
Since there are so many, I'm not sure if they should be corrected or not.

 

Thanks,

Weiming

 

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation

 

From: Jim Grosbach [mailto:grosbach at apple.com] 
Sent: Thursday, November 15, 2012 1:22 PM
To: weimingz at codeaurora.org
Cc: 'Jakob Stoklund Olesen'; llvm-commits at cs.uiuc.edu; zinob at codeaurora.org
Subject: Re: [llvm-commits] Fixing Bug 13662: paired register for inline asm
with 64-bit data on ARM

 

Hi Weiming,

 

A few stylistic notes are the only cleanups I'd like to see. The core code
is looking great. 

 

Jakob, do you have any additional changes you would like to see?

 

-Jim

 

+/// PairRRegs - Form a GPRPair pseudo register from a pair of G registers.

+///

 

The DOxygen style preferences have changed a bit recently. Have a look at
http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comme
nts for the gory details.

 

+SDNode *ARMDAGToDAGISel::PairGRegs(EVT VT, SDValue V0, SDValue V1) {

 

This function name could be clearer and should adhere to the coding syle
guidelines (start w/ a verb, camel-case w/ the first letter being lower
case). Something like createGPRPairNode(), perhaps.

 

+  DebugLoc dl = V0.getNode()->getDebugLoc();

+  SDValue RegClass =

+    CurDAG->getTargetConstant(ARM::GPRPairRegClassID, MVT::i32);

+  SDValue SubReg0 = CurDAG->getTargetConstant(ARM::gsub_0, MVT::i32);

+  SDValue SubReg1 = CurDAG->getTargetConstant(ARM::gsub_1, MVT::i32);

+  const SDValue Ops[] = { RegClass, V0, SubReg0, V1, SubReg1 };

+  return CurDAG->getMachineNode(TargetOpcode::REG_SEQUENCE, dl, VT, Ops,
5);

+}

 

--- a/lib/Target/ARM/ARMISelLowering.cpp

+++ b/lib/Target/ARM/ARMISelLowering.cpp

@@ -5771,12 +5771,16 @@ ARMTargetLowering::EmitAtomicBinary64(MachineInstr
*MI, MachineBasicBlock *BB,

   // for ldrexd must be different.

   BB = loopMBB;

   // Load

+  unsigned gpair0 = MRI.createVirtualRegister(&ARM::GPRPairRegClass);

+  unsigned gpair1 = MRI.createVirtualRegister(&ARM::GPRPairRegClass);

+

 

Variables should start with upper case. Perhaps RegPair0 and RegPair? Yes,
some of the surrounding code sets a bad example about this. :( Fixing some
of that as a separate follow-up patch would be very welcome (though in no
way required for this patch to go in).

 

+

+    unsigned undef = MRI.createVirtualRegister(&ARM::GPRPairRegClass);

 

Ditto. Perhaps UndefPair?

 

-Jim

 

 

On Nov 2, 2012, at 10:08 AM, Weiming Zhao <weimingz at codeaurora.org> wrote:





Hi Jim,

Thanks for your review and suggestion.
RegisterOperand does make the code cleaner.  I also add more explanations on
Disassembler and AsmParser.

Please help to review.

Thanks,
Weiming

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation


-----Original Message-----
From: Jim Grosbach [mailto:grosbach at apple.com] 
Sent: Thursday, November 01, 2012 5:16 PM
To: weimingz at codeaurora.org
Cc: 'Jakob Stoklund Olesen'; llvm-commits at cs.uiuc.edu; zinob at codeaurora.org
Subject: Re: [llvm-commits] Fixing Bug 13662: paired register for inline asm
with 64-bit data on ARM

Hi Weiming,

You may want to consider a RegisterOperand here, which allows a custom print
method on a register class. You can just use the RegisterOperand wherever
you'd normally use a RegisterClass in the (ins) and (outs) lists. For
example, def GPRPairOp : RegisterOperand<GPRPair, "printGPRPair">;

Then the printOperand() method doesn't need to perform the special "is this
a gprpair?" check for every register operand, which is a speed improvement
on the common path. There's a dedicated print method that's only used for
these operands.

The asm parser bit seems ok. It's a pretty ugly hack, but that's somewhat
necessary at this stage, as the parsers don't have any way to express these
kinds of constraints, so we have to do it manually in C++ code. Please add a
FIXME there with a very descriptive comment about why this is necessary.

-Jim

On Nov 1, 2012, at 3:35 PM, Weiming Zhao <weimingz at codeaurora.org> wrote:




Hi Jakob & Jim,

I updated the instruction def of ldrexd/strexd to let them use GPRPair 
Reg class.
I also fixed ldrexd/strexd intrinsics and atomic_64 lowering accordingly.
Disassembler and AsmParser are adjusted to fit the new instruction def.
The original test case "atomic-64bit.ll" is updated, so it's not 
checking hardcoded R0,R1 or R2,R3

It passes the "make check", especially atomic-64bit.ll, ldstrexd,ll, 
MC/ARM/basic-asm-instrucions.s, MC/ARM/diagnstics.s and 
MC/Disassembler/ARM/ basic-arm-instructions.txt

Please help to review it.

Thanks,
Weiming

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation


-----Original Message-----
From: Jim Grosbach [mailto:grosbach at apple.com]
Sent: Wednesday, October 31, 2012 3:24 PM
To: Jakob Stoklund Olesen
Cc: weimingz at codeaurora.org; llvm-commits at cs.uiuc.edu; 
zinob at codeaurora.org
Subject: Re: [llvm-commits] Fixing Bug 13662: paired register for 
inline asm with 64-bit data on ARM


On Oct 31, 2012, at 3:17 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk>

wrote:








On Oct 31, 2012, at 3:15 PM, "Weiming Zhao" <weimingz at codeaurora.org>

wrote:







Hi Jakob,

I'm changing the ldrexd/strexd of ARMInstrInfo.td to def LDREXD: 
AIldrex<0b01, (outs GPRPair:$Rt),(ins addr_offset_none:$addr),
                  NoItinerary, "ldrexd", "\t$Rt, $addr", []> { let 
DecoderMethod = "DecodeDoubleRegLoad"; }

def STREXD : AIstrex<0b01, (outs GPR:$Rd),
                (ins GPRPair:$Rt, addr_offset_none:$addr),
                NoItinerary, "strexd", "\t$Rd, $Rt, $addr", []> { 
let DecoderMethod = "DecodeDoubleRegStore"; }

Is this the way you're referring to?


Exactly.




So far, it almost works but I need to deal with printing "R0_R1" for 
the GPRPair reg name. I'm planning to do it in 
InstPrinter/ARMInstPrinter.cpp::printOperand().


I think that's right. Jim?


Sure, that'll work. Changing the instruction definitions like this 
will require some work in the AsmParser, too, though. I'm surprised 
you're not seeing a "make check" failure on basic-arm-instructions.s 
with these changes.

-Jim
<0002-Remove-hard-coded-registers-in-ARM-ldrexd-and-strexd.patch>


<0003-Remove-hard-coded-registers-in-ARM-ldrexd-and-strexd.patch>

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121116/0f638dcd/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: refactor.patch
Type: application/octet-stream
Size: 9738 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121116/0f638dcd/attachment.obj>

More information about the llvm-commits mailing list