<div dir="ltr">Yes, this *definitely* needs a test case. A little bit of pseudo-code in the commit message is not a useful test.<div>  <div><div>Also, please always use the proper syntax in your commit message to refer to the review. You need to have a line like this: "Differential Revision: <a href="http://reviews.llvm.org/D19368" rel="noreferrer" style="font-size:12.8px" target="_blank">http://reviews.llvm.org/D19368</a>" so that it will get associated with the review properly.</div></div><div><br></div><div>Finally, your commit message could be better. The first line should be a short summary of what you've done. Then, expand upon that in later paragraphs. </div><div><br></div><div>E.g.:</div><div>====</div><div>[SPARC] Fix DelaySlotFiller to handle ...whatever this fixes...</div></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">More lines of explanation why you need to do this. Blah blah</div><div class="gmail_extra">blah.</div><div><br></div></div><div class="gmail_extra">Differential Revision: <a href="http://reviews.llvm.org/D19368" rel="noreferrer" style="font-size:12.8px" target="_blank">http://reviews.llvm.org/D19368</a><br></div><div class="gmail_extra">====</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 22, 2016 at 1:57 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Why not add a testcase?<br>
<br>
Cheers,<br>
Rafael<br>
<br>
<br>
On 22 April 2016 at 04:13, Chris Dewhurst via llvm-commits<br>
<div><div><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: lerochris<br>
> Date: Fri Apr 22 03:13:47 2016<br>
> New Revision: 267120<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=267120&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=267120&view=rev</a><br>
> Log:<br>
> The following code would not work before this patch, due to the inability to take the address of a global object:<br>
><br>
> void func1() {<br>
><br>
> ...<br>
> }<br>
><br>
> int main(int argc, char** argv) {<br>
><br>
> void (*pFunc)();<br>
> pFunc = &func1<br>
> pFunc();<br>
> ...<br>
> }<br>
><br>
> Phabricator review: <a href="http://reviews.llvm.org/D19368" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19368</a><br>
><br>
> Modified:<br>
>     llvm/trunk/lib/Target/Sparc/DelaySlotFiller.cpp<br>
><br>
> Modified: llvm/trunk/lib/Target/Sparc/DelaySlotFiller.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Sparc/DelaySlotFiller.cpp?rev=267120&r1=267119&r2=267120&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Sparc/DelaySlotFiller.cpp?rev=267120&r1=267119&r2=267120&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Target/Sparc/DelaySlotFiller.cpp (original)<br>
> +++ llvm/trunk/lib/Target/Sparc/DelaySlotFiller.cpp Fri Apr 22 03:13:47 2016<br>
> @@ -296,7 +296,7 @@ void Filler::insertCallDefsUses(MachineB<br>
>      RegUses.insert(Reg.getReg());<br>
><br>
>      const MachineOperand &RegOrImm = MI->getOperand(1);<br>
> -    if (RegOrImm.isImm())<br>
> +    if (RegOrImm.isImm() || RegOrImm.isGlobal())<br>
>          break;<br>
>      assert(RegOrImm.isReg() && "CALLrr second operand is not a register.");<br>
>      assert(RegOrImm.isUse() && "CALLrr second operand is not a use.");<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>