review request Bug 16545 - COPY_STRUCT_BYVAL_I32 pseudo code generates unrecognized code

[Manman Ren]

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/e5748ae6/attachment.html>