review request Bug 16545 - COPY_STRUCT_BYVAL_I32 pseudo code generates unrecognized code

[David Peixotto]

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.

 

 

-- 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/72822deb/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: struct_byval_arm_t1_t2.ll
Type: application/octet-stream
Size: 44132 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131015/72822deb/attachment.obj>