[llvm] r213197 - trivial fix for PR20314

[Sanjay Patel]

Sorry about the slop. I blame existing code - everything I did was cut and
pasted from the code and testcases around this one. :)
Eg, there are 84 other pr* testcases in the test/Codegen/X86 dir.

I'll checkin with fixes again shortly, but I'm curious about this one:
"We don't usually put bug numbers in the comments.  Please remove."

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.

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.


On Wed, Jul 16, 2014 at 4:07 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2014-Jul-16, at 14:08, Sanjay Patel <spatel at rotateright.com> wrote:
> >
> > Author: spatel
> > Date: Wed Jul 16 16:08:10 2014
> > New Revision: 213197
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=213197&view=rev
> > Log:
> > trivial fix for PR20314
> >
> > Make sure that the AddrInst is an Instruction.
> >
>
> Thanks for fixing this!
>
> > Added:
> >    llvm/trunk/test/CodeGen/X86/pr20314.ll
> > Modified:
> >    llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp
> >
> > Modified: llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp?rev=213197&r1=213196&r2=213197&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp Wed Jul 16 16:08:10 2014
> > @@ -2130,8 +2130,11 @@ bool AddressingModeMatcher::MatchOperati
> >     return true;
> >   }
> >   case Instruction::SExt: {
> > +    // Make sure this isn't a ConstantExpr (PR20314).
>
> We don't usually put bug numbers in the comments.  Please remove.
>
> > +    Instruction *SExt = dyn_cast<Instruction>(AddrInst);
> > +    if (!SExt) return false;
>
> It's not in the coding standards, but clang-format prefers this return on
> the next line (I do too).
>
> > +
> >     // Try to move this sext out of the way of the addressing mode.
> > -    Instruction *SExt = cast<Instruction>(AddrInst);
> >     // Ask for a method for doing so.
> >     TypePromotionHelper::Action TPH = TypePromotionHelper::getAction(
> >         SExt, InsertedTruncs, TLI, PromotedInsts);
> >
> > Added: llvm/trunk/test/CodeGen/X86/pr20314.ll
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/pr20314.ll?rev=213197&view=auto
> >
> ==============================================================================
> > --- llvm/trunk/test/CodeGen/X86/pr20314.ll (added)
> > +++ llvm/trunk/test/CodeGen/X86/pr20314.ll Wed Jul 16 16:08:10 2014
>
> It'd be better to find a name that describes the situation.  Something
> like `address-type-promotion-constantexpr.ll`.
>
> > @@ -0,0 +1,14 @@
> > +; RUN: llc < %s -mtriple=x86_64-pc-linux
> > +
> > +; No check as PR20314 is a crashing bug.
>
> In general, it's best to add CHECK lines even for crashers if there's
> anything reasonable to check for.  Not sure what you'd add here, but
> think about whether there's something reasonable.
>
> BTW, listing the PR at the top here (as you've done) *is* a good idea.
>
> > +
> > + at c = common global [2 x i32] zeroinitializer, align 4
> > + at a = common global i32 0, align 4
> > + at b = internal unnamed_addr constant [2 x i8] c"\01\00", align 1
> > +
> > +define i32 @main() {
> > +entry:
> > +  %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
> > +  ret i32 0
> > +}
> > +
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>


-- 
Sanjay Patel
RotateRight, LLC
http://www.rotateright.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140716/0fcc83f3/attachment.html>