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

[Jim Grosbach]

The patch uses the register enum by value and assumes it is sorted by register number. This is not accurate. For example, ARM::SP has a lower enum value and ARM::R0.

Also note that the frame pointer is always R7 on Darwin.

-Jim

On Sep 17, 2012, at 4:57 PM, Weiming Zhao wrote:

> Hi Bill,
>  
> Thanks for the reviewing.
> -          Regarding parameter passing
> Actually, my first version was passing reference. Later on, I changed it to pass pointer since I can use NULL as the default parameter and thus minimize changes to existing code. Using reference,  I have to define a overloaded version that calls the existing version in TargetLower.h as the default implementation, and then override it in ARMISelLowering with my implementation. I can switch to passing reference in my next patch.
>  
> -          Regarding skipping R10 and comparing reg, I should add parenthesis. My intention is :
>                Reg != (Subtarget->isThumb() ? ARM::R6 : ARM::R10    My current code was ambiguous and probably wrong. I will fix it.    
>  
> -          I will check the find() and tGRPRegclass issue.
>  
> -          Regarding the return:
> when it can't find any available register, it returns "RCPair(0U, NULL);". This is expected behavior. I tested it by occupying almost all the GRPs, and in this case, Clang will return an error nicely:
> error: couldn't allocate input reg for constraint 'r'
>         __asm__ __volatile__("@ atomic64_set\n"
>  
> Again, thank you, Bill, for your comments and reviewing.
>  
> Thanks,
> Weiming
>  
>  
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>  
> -----Original Message-----
> From: Bill Wendling [mailto:wendling at apple.com] 
> Sent: Monday, September 17, 2012 3:03 PM
> To: Weiming Zhao
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] Fixing Bug 13662: paired register for inline asm with 64-bit data on ARM
>  
> Hi Weiming,
>  
> One thing I noticed in your patch, you're passing around a SmallVector as a pointer. Why are you doing that? Instead, you should just pass it by reference:
>  
>      getRegForInlineAsmConstraint(const std::string &Constraint,
>                                   EVT VT,
>                                   SmallVectorImpl<unsigned> &AssignedPhyRegs) const;
>  
> etc. It doesn't make sense to have it be a pointer since you're not testing to make sure it's not NULL before you're dereferencing it.
>  
> Also, this logic is weird:
>  
> +      if (VT.getSizeInBits() == 64 && AssignedPhyRegs) {
> +        SmallVectorImpl<unsigned>::iterator First = AssignedPhyRegs->begin(),
> +          Last = AssignedPhyRegs->end();
> +        // The valid registers for ldrexd/strexd is r0-r13 for ARM. But we skip
> +        // the pairs using FP(r7/r11 for T1/ARM) and SP(r13) for safety.
>  
> This comment is misleading. We're not skipping R11 for ARM because you have 'Reg <= ARM::R10', which may choose 'R10/R11'.
>  
> +        for (unsigned Reg = ARM::R0; Reg <= ARM::R10 &&
> +            Reg != Subtarget->isThumb() ? ARM::R6 : ARM::R10; Reg += 2)
>  
> Why are you comparing 'Reg', an unsigned, to 'Subtarget->isThumb()', a bool?
>  
> +          if (std::find(First, Last, Reg) == Last &&
> +              std::find(First, Last, Reg + 1) == Last)
>  
> This is expensive. Is there another way of doing this without the find? Like maybe creating a bit vector of assigned physregs and then looking at that? (Just for an example...you can come up with your own idea.)
>  
> +            return RCPair(Reg, &ARM::GPRRegClass);
>  
> I think that in Thumb mode, you want ARM::tGPRRegClass instead of ARM::GPRRegClass. (Can anyone confirm this?)
>  
> +        return RCPair(0U, NULL);
>  
> Hork? Is this the correct action here? What does this do exactly?
>  
> +      }
>  
>  
> -bw
>  
> On Sep 17, 2012, at 2:32 PM, Weiming Zhao <weimingz at codeaurora.org> wrote:
>  
> > Ping…
> > 
> > From: Weiming Zhao [mailto:weimingz at codeaurora.org]
> > Sent: Wednesday, September 12, 2012 10:21 AM
> > To: llvm-commits at cs.uiuc.edu
> > Subject: Fixing Bug 13662: paired register for inline asm with 64-bit data on ARM
> > 
> > Hi,
> > 
> > Attached is the patch for fixing the long long data passing to inline asm for ARM.
> > 
> > Since current LLVM has no support for paired GPR reg class for 64-bit data, we follow the same practice as ldrexd/strexd instincs (ARMTargetLowering::EmitAtomicBinary64).  That is, we hard code the physical registers.
> > 
> > However, since inline asm may has some physical registers specified by programmers, we cannot simply hard code R2/R3 as those intrincs do.
> > So, we have to add a variable “AssignedPhyRegs” to track those already specified physical regs of that inline ASM and thus avoid them during assigning.
> > 
> > Please help to review the patch.
> > 
> > Thanks,
> > Weiming
> > 
> > 
> > 
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> > 
> > <0001-Bug-13622-Fix-paired-register-for-inline-asm-with-64.patch>_______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>  
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

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