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

Jim Grosbach grosbach at apple.com
Fri Nov 16 15:45:06 PST 2012


Hi Weiming,

This looks great, please apply.

The Select* methods can stay as-is for now. That interface is old and predates the convention. It should be cleaned up, but it's not a big deal for now.

Thanks again for your previous patch and for this follow up.

-Jim

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

> 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-comments 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>
>  
> <refactor.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121116/58c8c4ab/attachment.html>


More information about the llvm-commits mailing list