review request Bug 16545 - COPY_STRUCT_BYVAL_I32 pseudo code generates unrecognized code

David Peixotto dpeixott at codeaurora.org
Tue Oct 15 15:27:06 PDT 2013


Hi Manman,

 

I split the patch into two parts as requested. Most of the changes are in the refactoring patch. The thumb1 support is pretty straightforward after the refactoring is done.

 

Thanks for your help reviewing these patches.

 

 

 

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

 

 

From: Manman Ren [mailto:manman.ren at gmail.com] 
Sent: Tuesday, October 15, 2013 2:01 PM
To: David Peixotto
Cc: 林作健; llvm-commits
Subject: Re: review request Bug 16545 - COPY_STRUCT_BYVAL_I32 pseudo code generates unrecognized code

 

 

Hi David,

 

Thanks for working on this.

Can you separate your patch to a refactoring patch and a patch for Thumb1?

 

It is easier to review and we should submit it as two commits.

 

Manman

 

On Tue, Oct 15, 2013 at 1:16 PM, David Peixotto <dpeixott at codeaurora.org> wrote:

I’ve attached a patch that fixes the issue for thumb1 and thumb2. I refactored the code a bit to isolate all the target-specific to a helper class. It makes the code a bit longer, but it helped me to see where we need different operations for each subtarget and to implement them in isolation. I’m not opposed to doing it all inline, but I wasn’t sure I would be able to get all the details right that way.

 

This commit refactors the code so that we can easily support thumb1

targets and implements the thumb1 lowering.

 

The original lowering algorithm is unchanged, but it now uses a

helper class to generate the machine instructions.  The refactoring

introduces a StructByvalEmitter class that encapsulates the

operations needed to lower the COPY_STRUCT pseudo instruction. The

basic operations (e.g. emit load, emit store) are all wrappers to

the correct machine instruction for each target. The operations are

then defined for Arm, Thumb1, and Thumb2 targets.

 

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

 

 

From: Manman Ren [mailto:manman.ren at gmail.com] 
Sent: Tuesday, October 15, 2013 11:51 AM
To: David Peixotto
Cc: 林作健; llvm-commits


Subject: Re: review request Bug 16545 - COPY_STRUCT_BYVAL_I32 pseudo code generates unrecognized code

 

 

 

On Tue, Oct 15, 2013 at 10:13 AM, David Peixotto <dpeixott at codeaurora.org> wrote:

Why does this patch need to make a change to the ARMConstantIslandsPass? I think it may be because we are still using an arm instruction for the constant load instead of the equivalent thumb instruction (tLDRpci).

 

I think your patch is incomplete for supporting thumb1. You need to modify the lowering for COPY_STRUCT_BYVAL_I32 in a few more places. We should be using the thumb1 opcodes for all the instructions generated by the lowering. You have handled the load/store case for the values, but you still need to take care of the constant pool load, the subtract (for the loop counter) and the branch. These instructions should use the tLDRpci (for constant pool load), tSubi8 (for the loop counter subtract), and tBcc (for the loop branch).

 

I didn’t see your bug and had filed another bug for this same issue: http://llvm.org/bugs/show_bug.cgi?id=17309. Please also check that your patch works with the test case I uploaded to that bug (also attached to this email). In addition to checks for thumb1, this test case also exposes a bug in the current code with the thumb2 lowering. It will trigger an assertion because an operand is added to a full instruction. It looks like the thumb2 failure comes from a copy/paste error when building the machine instructions.

 

Thanks for the testing case. Yes, the thumb2 failure is from a copy/pasto. I will fix that.

 

Manman 

 

 

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

 

 

From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of ???
Sent: Monday, October 14, 2013 6:29 PM
To: Manman Ren
Cc: llvm-commits
Subject: Re: review request Bug 16545 - COPY_STRUCT_BYVAL_I32 pseudo code generates unrecognized code

 

I didn't add THUMB1 lines for function h simple armv5te doesn't support vldr instr.

And here is the patch for formatting.

 

2013/10/15 Manman Ren <manman.ren at gmail.com>

Hi,

 

The changes to ARMISelLowering.cpp look good to me.

 

--- lib/Target/ARM/ARMConstantIslandPass.cpp (版本 187190)

+++ lib/Target/ARM/ARMConstantIslandPass.cpp (工作副本)

@@ -634,6 +634,7 @@

 initializeFunctionInfo(const std::vector<MachineInstr*> &CPEMIs) {

   BBInfo.clear();

   BBInfo.resize(MF->getNumBlockIDs());

+  const ARMSubtarget * Subtarget = &MF->getTarget().getSubtarget<ARMSubtarget>();

 

   // First thing, compute the size of all basic blocks, and see if the function

   // has any inline assembly in it. If so, we have to be conservative about

@@ -757,7 +758,12 @@

           case ARM::LDRi12:

           case ARM::LDRcp:

           case ARM::t2LDRpci:

-            Bits = 12;  // +-offset_12

+            if(Subtarget->hasV7Ops())

+            {

+              Bits = 12;  // +-offset_12

+            } else {

+              Bits = 8;  // +-offset_8

+            }

 

Please simplify above by removing the parenthesis, or using "? 12 : 8".

I am not really familiar with the const island pass, so I cc'ed Bob for him to review it.

 

The testing case looks good in general, is there any reason you didn't add THUMB1 lines for function h()?

 

Thanks,

Manman

 

 

On Sun, Oct 13, 2013 at 4:48 AM, 林作健 <manjian2006 at gmail.com> wrote:

 


_______________________________________________
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/20131015/ef824a84/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Refactor-lowering-for-COPY_STRUCT_BYVAL_I32.patch
Type: application/octet-stream
Size: 28551 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131015/ef824a84/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-PR17309-ARM-backend-incorrectly-lowers-COPY_STRUCT_B.patch
Type: application/octet-stream
Size: 55877 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131015/ef824a84/attachment-0001.obj>


More information about the llvm-commits mailing list