[llvm-commits] [llvm] r166607 - in /llvm/trunk: include/llvm/DataLayout.h lib/Analysis/ConstantFolding.cpp lib/Analysis/InlineCost.cpp lib/Analysis/InstructionSimplify.cpp lib/CodeGen/AsmPrinter/AsmPrinter.cpp lib/ExecutionEngine/ExecutionEngine.

[Villmow, Micah]

> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Chandler Carruth
> Sent: Wednesday, October 24, 2012 2:01 PM
> To: Micah Villmow
> Cc: Commit Messages and Patches for LLVM
> Subject: Re: [llvm-commits] [llvm] r166607 - in /llvm/trunk:
> include/llvm/DataLayout.h lib/Analysis/ConstantFolding.cpp
> lib/Analysis/InlineCost.cpp lib/Analysis/InstructionSimplify.cpp
> lib/CodeGen/AsmPrinter/AsmPrinter.cpp
> lib/ExecutionEngine/ExecutionEngine.
> 
> On Wed, Oct 24, 2012 at 11:36 AM, Micah Villmow <villmow at gmail.com>
> wrote:
> > Author: mvillmow
> > Date: Wed Oct 24 13:36:13 2012
> > New Revision: 166607
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=166607&view=rev
> > Log:
> > Add some cleanup to the DataLayout changes requested by Chandler.
> 
> And you've introduced quite a few really bad style violations. Please
> review your patch for style before committing, or ask someone else
> to...
> 
> 
> >
> =======================================================================
> =======
> > --- llvm/trunk/include/llvm/DataLayout.h (original)
> > +++ llvm/trunk/include/llvm/DataLayout.h Wed Oct 24 13:36:13 2012
> > @@ -262,6 +262,14 @@
> >      }
> >      return 8*val->second.TypeBitWidth;
> >    }
> 
> Vertical whitespace here.
[Villmow, Micah] Not sure what you mean here, do you want newlines after the last brace?
> 
> > +  /// Layout pointer size, in bits, based on the type.
> > +  /// If this function is called with a pointer type, then
> > +  /// the type size of the pointer is returned.
> > +  /// If this function is called with a vector of pointers,
> > +  /// then the type size of the pointer is returned.
> > +  /// Otherwise the type sizeo f a default pointer is returned.
> 
> If you're going to write a doxygen comment (and I ask that you do!) at
> least use the doxygen structure. \brief and \returns would help this a
> lot.
> 
> > +  unsigned getPointerTypeSizeInBits(Type* Ty)    const;
> 
> "Type *Ty" please, and only one space before the 'const'....
> 
> And then...
> 
> > -    unsigned AS = GV->getType()->getAddressSpace();
> > -    unsigned PtrSize = TD->getPointerSizeInBits(AS)/8;
> > +    assert(GV->getType()->isPointerTy() && "GV must be a pointer
> type!");
> > +    unsigned PtrSize = TD->getTypeSizeInBits(GV->getType())/8;
> 
> Rather than an assert at each call site, why not just call
> getPointerTypeSizeInBits(GV->getType()) and let that function assert?
[Villmow, Micah] getPointerTypeSizeInBits in the case where the value is not
a pointer returns the size of the default address space. I'll update the
documentation in the function to make this more clear.
> 
> > +      assert(CE->getOperand(1)->getType()->isPointerTy() &&
> > +          "Must be a pointer type!");
> 
> And many of the asserts you've added are incorrectly indented such as
> this one.
[Villmow, Micah] Will fix this up.
> 
> > +unsigned DataLayout::getPointerTypeSizeInBits(Type *Ty) const
> > +{
> 
> No, {s go on the previous line, always.
> 
> > +    if (Ty->isPointerTy()) return getTypeSizeInBits(Ty);
> > +    if (Ty->isVectorTy()
> > +        && cast<VectorType>(Ty)->getElementType()->isPointerTy())
> 
> Huh? cast<>() returns a pointer, always. It will assert if it is given
> a null pointer or the type doesn't match.
[Villmow, Micah] Not sure what you mean here, this is for handling the case of <2 x i8*>.
> 
> > +      return getTypeSizeInBits(cast<VectorType>(Ty)-
> >getElementType());
> > +    return getPointerSizeInBits(0);
> 
> And this just doesn't make sense... Why do you allow non-pointer types
> as arguments?
[Villmow, Micah] There were lots of places in the code base where there was not a guarantee that the type being evaluated for the pointer size was a pointer type. In some places it was a vector of pointers, in others the value of 'void' would show up or a label. In these cases, the size of the default pointer is all that is required, so instead of complicating the caller, I moved the logic into the callee to simplify things.
> 
> I think this function should be:
> 
> if (VectorType *VTy = dyn_cast<VectorType>(Ty))
>   Ty = VTy->getElementType();
> return getTypeSizeInBits(cast<PointerType>(Ty))
[Villmow, Micah] I'll restructure it based on this feedback, but I still think this function needs to handle the case of non-pointer/vec of pointer type.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits