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

[Weiming Zhao]

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 < <mailto:weimingz at codeaurora.org>
weimingz at codeaurora.org> wrote:

 

> Ping.

>  

> From: Weiming Zhao  <mailto:[mailto:weimingz at codeaurora.org]>
[mailto:weimingz at codeaurora.org] 

> Sent: Wednesday, September 12, 2012 10:21 AM

> To:  <mailto:llvm-commits at cs.uiuc.edu> 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

>  <mailto:llvm-commits at cs.uiuc.edu> llvm-commits at cs.uiuc.edu

>  <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
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/20120917/cc3e6c90/attachment.html>