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

[Weiming Zhao]

Hi,

If there is no objection, how about let's move ahead? 

Let's wait till tomorrow, if no objections I plan to check in then.

Thanks,
Weiming

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


-----Original Message-----
From: Weiming Zhao [mailto:weimingz at codeaurora.org] 
Sent: Friday, November 02, 2012 10:09 AM
To: 'Jim Grosbach'
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 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>