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

[Jim Grosbach]

Nope, not neglected. Just crazy busy reviewer schedules. I'm still very interested in getting this in, just needs Jakob and I to get some time free to look at the latest.

-j
On Nov 15, 2012, at 11:08 AM, Weiming Zhao <weimingz at codeaurora.org> wrote:

> HI Jim,
> 
> I definitely agree with you.  
> I was worried that the patch was neglected and now I'm gladly to find that
> it was not the case :-) 
> 
> 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 15, 2012 9:58 AM
> To: weimingz at codeaurora.org
> Cc: 'Jakob Stoklund Olesen'; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] Fixing Bug 13662: paired register for inline asm
> with 64-bit data on ARM
> 
> 
> On Nov 15, 2012, at 9:38 AM, Weiming Zhao <weimingz at codeaurora.org> wrote:
> 
>> 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.
>> 
> 
> That's not how the review process works. Do not do commit without explicit
> buy-in.
> 
> -Jim
> 
>> 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>
>> 
>> 
> 
>