[PATCH] Beginning of alloca implementation for Mips fast-isel

[Eric Christopher]

On Fri Feb 13 2015 at 4:00:39 PM reed kotler <rkotler at mips.com> wrote:

>  Subtarget is initialized first and then TargetSupported.
>
> It may have been a bad choice of name but TargetSupported is derived from
> Subtarget.
> That name has been in Mips fast-isel for a long time now. It's a local
> variable in the mips fast-isel class.
>
> Probably should have been called SubtargetSupported.
>
>
>
Rather than write it out again, how about you look at the patch that
changed the code that you're trying to change back?

-eric


> On 02/13/2015 03:50 PM, Eric Christopher wrote:
>
>
>
> On Fri Feb 13 2015 at 3:47:23 PM reed kotler <rkotler at mips.com> wrote:
>
>> Not sure I understand some of these comments.
>>
>> On 02/13/2015 03:21 PM, Eric Christopher wrote:
>> > Hi Reed,
>> >
>> > Some quick inline comments.
>> >
>> > +      assert(isFIBase() && "Invalid base frame index  access!");
>> >
>> > formatting.
>> This how clang format does it. What do you want?
>>
>
>  You have an extra space between index and access.
>
>
>> > - const MipsSubtarget *Subtarget; const TargetInstrInfo &TII; const
>> TargetLowering &TLI;
>> >
>> > +  const MipsSubtarget *Subtarget;
>> >
>> > Unnecessary.
>> >
>> > +        Subtarget(&TM.getSubtarget<MipsSubtarget>()) {
>> >
>> > No.
>> >
>> > +    TargetSupported = ((Subtarget->getRelocationModel() ==
>> Reloc::PIC_) &&
>> > +                       ((Subtarget->hasMips32r2() ||
>> > Subtarget->hasMips32()) &&
>> > +                        (Subtarget->isABI_O32())));
>> >
>> > Ditto.
>>
>> What is the issue here?
>>
>>
>  You've moved where subtarget is initialized, and changed (in the wrong
> way) how Subtarget and TargetSupported are initialized.
>
>
>> Formatting?
>>
>> This is what clang format does.
>> >
>> > +  // This code is mostly cloned from AArch64 (which cloned it from
>> earlier
>> > +  // ports)'
>> >
>> > Unnecessary.
>> You mean the comment is not necessary?
>>
>
>  Correct.
>
>
>> >
>> > +    unsigned Offset = Addr.getOffset();
>> > +    ;
>> > +    MachineFrameInfo &MFI = *MF->getFrameInfo();
>> > +    ;
>> >
>> > ?
>> You mean the extra ";" lines?
>> Hmmm.. probably from clang format but I don't know.
>>
>
>  clang-format would not have put them there. It doesn't insert characters.
>
>
>> Looks wrong.
>>
>
>  Yep.
>
>  -eric
>
>
>>
>> > -eric
>> >
>> >
>> > http://reviews.llvm.org/D6426
>> >
>> > EMAIL PREFERENCES
>> >    http://reviews.llvm.org/settings/panel/emailpreferences/
>> >
>> >
>>
>> _______________________________________________
>> 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/20150214/f6f33bd3/attachment.html>