<div dir="ltr"><div><div>Sorry about the slop. I blame existing code - everything I did was cut and pasted from the code and testcases around this one. :)<br></div><div>Eg, there are 84 other pr* testcases in the test/Codegen/X86 dir.<br>
<br></div>I'll checkin with fixes again shortly, but I'm curious about this one:<br>"We don't usually put bug numbers in the comments.  Please remove."<br><br></div><div>This fix is too easy and I agree, but for larger bug fixes that I've made over the last few weeks, I've referenced bug numbers in comments. I think that's for the best for whoever wanders into that code blindly next. I'm purposely being very verbose in bugzilla so others can clearly see how the code came to be.<br>
<br>I'm new to this codebase, and I'm cursing the lack of comments and external references for big blocks of logic that I'm trying to debug.<br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Wed, Jul 16, 2014 at 4:07 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> On 2014-Jul-16, at 14:08, Sanjay Patel <<a href="mailto:spatel@rotateright.com">spatel@rotateright.com</a>> wrote:<br>
><br>
> Author: spatel<br>
> Date: Wed Jul 16 16:08:10 2014<br>
> New Revision: 213197<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=213197&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=213197&view=rev</a><br>
> Log:<br>
> trivial fix for PR20314<br>
><br>
> Make sure that the AddrInst is an Instruction.<br>
><br>
<br>
</div>Thanks for fixing this!<br>
<div class=""><br>
> Added:<br>
>    llvm/trunk/test/CodeGen/X86/pr20314.ll<br>
> Modified:<br>
>    llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp<br>
><br>
> Modified: llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp?rev=213197&r1=213196&r2=213197&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp?rev=213197&r1=213196&r2=213197&view=diff</a><br>

> ==============================================================================<br>
> --- llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp (original)<br>
> +++ llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp Wed Jul 16 16:08:10 2014<br>
> @@ -2130,8 +2130,11 @@ bool AddressingModeMatcher::MatchOperati<br>
>     return true;<br>
>   }<br>
>   case Instruction::SExt: {<br>
> +    // Make sure this isn't a ConstantExpr (PR20314).<br>
<br>
</div>We don't usually put bug numbers in the comments.  Please remove.<br>
<div class=""><br>
> +    Instruction *SExt = dyn_cast<Instruction>(AddrInst);<br>
> +    if (!SExt) return false;<br>
<br>
</div>It's not in the coding standards, but clang-format prefers this return on<br>
the next line (I do too).<br>
<div class=""><br>
> +<br>
>     // Try to move this sext out of the way of the addressing mode.<br>
> -    Instruction *SExt = cast<Instruction>(AddrInst);<br>
>     // Ask for a method for doing so.<br>
>     TypePromotionHelper::Action TPH = TypePromotionHelper::getAction(<br>
>         SExt, InsertedTruncs, TLI, PromotedInsts);<br>
><br>
> Added: llvm/trunk/test/CodeGen/X86/pr20314.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/pr20314.ll?rev=213197&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/pr20314.ll?rev=213197&view=auto</a><br>

> ==============================================================================<br>
> --- llvm/trunk/test/CodeGen/X86/pr20314.ll (added)<br>
> +++ llvm/trunk/test/CodeGen/X86/pr20314.ll Wed Jul 16 16:08:10 2014<br>
<br>
</div>It'd be better to find a name that describes the situation.  Something<br>
like `address-type-promotion-constantexpr.ll`.<br>
<div class=""><br>
> @@ -0,0 +1,14 @@<br>
> +; RUN: llc < %s -mtriple=x86_64-pc-linux<br>
> +<br>
> +; No check as PR20314 is a crashing bug.<br>
<br>
</div>In general, it's best to add CHECK lines even for crashers if there's<br>
anything reasonable to check for.  Not sure what you'd add here, but<br>
think about whether there's something reasonable.<br>
<br>
BTW, listing the PR at the top here (as you've done) *is* a good idea.<br>
<div class="HOEnZb"><div class="h5"><br>
> +<br>
> +@c = common global [2 x i32] zeroinitializer, align 4<br>
> +@a = common global i32 0, align 4<br>
> +@b = internal unnamed_addr constant [2 x i8] c"\01\00", align 1<br>
> +<br>
> +define i32 @main() {<br>
> +entry:<br>
> +  %foo = load i8* getelementptr ([2 x i8]* @b, i64 0, i64 sext (i8 or (i8 zext (i1 icmp eq (i32* getelementptr inbounds ([2 x i32]* @c, i64 0, i64 1), i32* @a) to i8), i8 1) to i64)), align 1<br>
> +  ret i32 0<br>
> +}<br>
> +<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">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/mailman/listinfo/llvm-commits</a><br>
<br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br>Sanjay Patel<br>RotateRight, LLC<br><a href="http://www.rotateright.com">http://www.rotateright.com</a>
</div>