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

[Bill Wendling]

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