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

Mon Oct 1 14:47:36 PDT 2012

Hi Jim,

Actually, we considered this approach before. However, we find it requires
non-trivial changes.

The main issues is that we don't want all long long data to be bounded to
this reg class because it is unnecessary for the majority 64-bits ARM
instructions , e. g. ,add.i64 because they don't require consecutive GPRs. 

To deal with this new issue, we would have to let type legalization pass
etc. to differentiate these cases.  So we actually  transfer the complexity
to here. 

Besides, to support the new 'virtual' reg class, we need to change a lot of
related classes like AsmWriter,  RegAlloc etc.

So, considering the high implementation cost, stability risk and limited
profit (just for ldrexd/strexd cases), we opted to follow the same design
approach as EmitAtomicBinary64 () and Intrinsics::arm_ldrexd() do. 

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: Monday, October 01, 2012 8:40 AM
To: Weiming Zhao
Cc: Bill Wendling; llvm-commits at cs.uiuc.edu LLVM; Bob Wilson
Subject: Re: [llvm-commits] Fixing Bug 13662: paired register for inline asm
with 64-bit data on ARM

Hello,

I'm a bit confused by the following FIXME from the patch:

+      // FIXME: When LLVM supports paired register class for 64-bit data in

+      // future, we should just return that register class here and let
register

+      // allocator to assign physical registers.

Why not just add such a register class and use it? It seems that would make
all of the AssignedPhysRegs handling unnecessary.

-Jim

On Sep 21, 2012, at 2:57 PM, Weiming Zhao <weimingz at codeaurora.org> wrote:

Ping ?

From: llvm-commits-bounces at cs.uiuc.edu
[mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Weiming Zhao
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:[mailto:grosbach at apple.com]>
[mailto:grosbach at apple.com] 
Sent: Tuesday, September 18, 2012 12:06 PM
To: Weiming Zhao
Cc: 'Bill Wendling';  <mailto:llvm-commits at cs.uiuc.edu>
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:[mailto:wendling at apple.com]>
[mailto:wendling at apple.com] 
Sent: Monday, September 17, 2012 3:03 PM
To: Weiming Zhao
Cc:  <mailto:llvm-commits at cs.uiuc.edu> 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
 <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/20121001/078d8833/attachment.html>