[llvm-commits] [llvm] r165126 - /llvm/trunk/lib/Transforms/Utils/BypassSlowDivision.cpp

[Nowicki, Tyler]

Thank you for describing how contexts are used. This was previously a mystery to me. This optimization is not applied to vectors and I'm not glued to using types, they just seemed like a good idea at the time.

I've removed the uses of global contexts and types as you suggested. The commit is r165255.

Thanks,

Tyler

> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Nick Lewycky
> Sent: Thursday, October 04, 2012 4:37 AM
> To: Gurd, Preston
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [llvm] r165126 -
> /llvm/trunk/lib/Transforms/Utils/BypassSlowDivision.cpp
> 
> Preston Gurd wrote:
> > Author: pgurd
> > Date: Wed Oct  3 11:11:44 2012
> > New Revision: 165126
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=165126&view=rev
> > Log:
> > This Patch corrects a problem whereby the optimization to use a faster
> > divide instruction (for Intel Atom) was not being done by Clang,
> > because the type context used by Clang is not the default context.
> 
> No, this is wrong. Please revert this patch.
> 
> The point of having multiple contexts is to permit two users of llvm-as-a-
> library in a single program to work without interfering with each other. Even
> just converting an integer type from context A to a different context would
> cause a data race between this and another thread that's using the global
> context.
> 
> There's a special exception for the ilist end iterator using getGlobalContext(),
> but pretty much anything else that isn't the top-level program calling
> getGlobalContext() is a bug.
> 
> I think the bug is in lib/Target/X86/X86ISelLowering.cpp where we add the
> slow bypass types using getGlobalContext(). And I have bad news, which is
> that you don't have the context at the point where your code that neesd it
> currently is (and indeed, none of the rest of the constructor relies on having
> it either). Is there a reason you need
> Type* in BypassSlowDivTypes, or could you just have the bitwidth of
> integers? (ie., do you care about vectors?)
> 
> Nick
> 
> > It fixes the problem by getting the global context types for each
> > div/rem instruction in order to compare them against the types in the
> BypassTypeMap.
> >
> > Tests for this will be done as a separate patch to Clang.
> >
> > Patch by Tyler Nowicki.
> >
> >
> > Modified:
> >      llvm/trunk/lib/Transforms/Utils/BypassSlowDivision.cpp
> >
> > Modified: llvm/trunk/lib/Transforms/Utils/BypassSlowDivision.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/By
> > passSlowDivision.cpp?rev=165126&r1=165125&r2=165126&view=diff
> >
> ==============================================================
> ========
> > ========
> > --- llvm/trunk/lib/Transforms/Utils/BypassSlowDivision.cpp (original)
> > +++ llvm/trunk/lib/Transforms/Utils/BypassSlowDivision.cpp Wed Oct  3
> > +++ 11:11:44 2012
> > @@ -238,14 +238,24 @@
> >       if (!UseDivOp&&  !UseRemOp)
> >         continue;
> >
> > +    // Skip division on vector types, only optimize integer instructions
> > +    if (!J->getType()->isIntegerTy())
> > +      continue;
> > +
> > +    // Get same type in global context
> > +    IntegerType *T = cast<IntegerType>(J->getType());
> > +    IntegerType *GT = IntegerType::get(getGlobalContext(),
> > + T->getBitWidth());
> > +
> >       // Continue if div/rem type is not bypassed
> > -    DenseMap<Type *, Type *>::const_iterator BT =
> > -      BypassTypeMap.find(J->getType());
> > -    if (BT == BypassTypeMap.end())
> > +    DenseMap<Type *, Type *>::const_iterator BI =
> BypassTypeMap.find(GT);
> > +    if (BI == BypassTypeMap.end())
> >         continue;
> >
> > -    IntegerType *BypassType = cast<IntegerType>(BT->second);
> > -    MadeChange |= reuseOrInsertFastDiv(F, I, J, BypassType, UseDivOp,
> > +    // Get the bypass type in the original context
> > +    IntegerType *GBT = cast<IntegerType>(BI->second);
> > +    IntegerType *BT = IntegerType::get(J->getContext(),
> > + GBT->getBitWidth());
> > +
> > +    MadeChange |= reuseOrInsertFastDiv(F, I, J, BT, UseDivOp,
> >                                          UseSignedOp, DivCache);
> >     }
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits