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

Wed Sep 26 13:19:33 PDT 2012

Ping?

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

From: Weiming Zhao [mailto:weimingz at codeaurora.org] 
Sent: Tuesday, September 18, 2012 3:01 PM
To: 'Jim Grosbach'; 'Bill Wendling'
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,

I address the issues that Jim and Bill mentioned:

- Changed to passing by reference.

- Fixed the ambiguous comparing logic.

- Changed SmallVector to SmallSet for quick test. I didn't use BitVector as
it often needs to be resized to the largest reg enum value.

- It returns ARM::GPRRegclass for both ARM and Thumb because 'r' modifiers
doesn't differentiate ARM/Thumb registers. ("l" and "h" does that)

- It uses an array to specify the register allocation order explicitly, not
relying on the enum value.

- For FP register, now R7 is always skipped. 

Please help to review the attached patch.

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: Tuesday, September 18, 2012 12:06 PM
To: Weiming Zhao
Cc: 'Bill Wendling'; llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] Fixing Bug 13662: paired register for inline asm
with 64-bit data on ARM

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 < <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

_______________________________________________
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/20120926/958e7c35/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Bug-13622-Fix-paired-register-for-inline-asm-with-64.patch
Type: application/octet-stream
Size: 13386 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120926/958e7c35/attachment.obj>