<div dir="ltr"><br><br><div class="gmail_quote">On Fri Feb 13 2015 at 3:47:23 PM reed kotler <<a href="mailto:rkotler@mips.com">rkotler@mips.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Not sure I understand some of these comments.<br>
<br>
On 02/13/2015 03:21 PM, Eric Christopher wrote:<br>
> Hi Reed,<br>
><br>
> Some quick inline comments.<br>
><br>
> +      assert(isFIBase() && "Invalid base frame index  access!");<br>
><br>
> formatting.<br>
This how clang format does it. What do you want?<br></blockquote><div><br></div><div>You have an extra space between index and access.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> - const MipsSubtarget *Subtarget; const TargetInstrInfo &TII; const TargetLowering &TLI;<br>
><br>
> +  const MipsSubtarget *Subtarget;<br>
><br>
> Unnecessary.<br>
><br>
> +        Subtarget(&TM.getSubtarget<<u></u>MipsSubtarget>()) {<br>
><br>
> No.<br>
><br>
> +    TargetSupported = ((Subtarget-><u></u>getRelocationModel() == Reloc::PIC_) &&<br>
> +                       ((Subtarget->hasMips32r2() ||<br>
> Subtarget->hasMips32()) &&<br>
> +                        (Subtarget->isABI_O32())));<br>
><br>
> Ditto.<br>
<br>
What is the issue here?<br>
<br></blockquote><div><br></div><div>You've moved where subtarget is initialized, and changed (in the wrong way) how Subtarget and TargetSupported are initialized.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Formatting?<br>
<br>
This is what clang format does.<br>
><br>
> +  // This code is mostly cloned from AArch64 (which cloned it from earlier<br>
> +  // ports)'<br>
><br>
> Unnecessary.<br>
You mean the comment is not necessary?<br></blockquote><div><br></div><div>Correct.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> +    unsigned Offset = Addr.getOffset();<br>
> +    ;<br>
> +    MachineFrameInfo &MFI = *MF->getFrameInfo();<br>
> +    ;<br>
><br>
> ?<br>
You mean the extra ";" lines?<br>
Hmmm.. probably from clang format but I don't know.<br></blockquote><div><br></div><div>clang-format would not have put them there. It doesn't insert characters.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Looks wrong.<br></blockquote><div><br></div><div>Yep.</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> -eric<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D6426" target="_blank">http://reviews.llvm.org/D6426</a><br>
><br>
> EMAIL PREFERENCES<br>
>    <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/<u></u>settings/panel/<u></u>emailpreferences/</a><br>
><br>
><br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>