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

[reed kotler]

I see that you and Chandler made some changes in this area.

I will go back and make sure that this change does not undo those.

On 02/13/2015 04:05 PM, Eric Christopher wrote:
>
>
> On Fri Feb 13 2015 at 4:00:39 PM reed kotler <rkotler at mips.com 
> <mailto: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
>>     <mailto: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 <mailto: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/20150213/ce15c100/attachment.html>