[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.

[Chandler Carruth]

On Wed, Oct 24, 2012 at 2:28 PM, Villmow, Micah <Micah.Villmow at amd.com> wrote:
>
>
>> -----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?

Uh, yes. Like essentially every other close brace after a function
definition in the code base?

>>
>> > +  /// 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.

I realized this when I got to the definition. I think that's wrong, so
let's go discuss it down there.

>>
>> > +      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*>.

Ok, so you're just testing for vector type redundantly three times.

The idiom is always to combine the cast and the test into a single
operation when you need both. That is why dyn_cast exists.

>>
>> > +      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.

If this function is intended to handle cases where there is no
guarantee that we have a pointer type, because that is the dominant
pattern, then it should have been committed in the same commit that
updates these "lots of places" to use it. The uses I see in this
commit would all be substantially simpler with the definition and
semantics I suggested.

I also can't really figure out in what case you would actually not
know whether or not the type you have is a pointer type, and yet want
a pointer size which is in some way "based" on it... It's just really
unclear what the use case for this could possibly be. On the other
hand, having a routine that assumes (and checks) its argument is a
pointer-like type (either a pointer or a vector of pointers) and
extracts the relevant pointer size from that, seems quite useful.


I suggest you take one of two approaches:

1) Fix the code you have touched here to be really simple by making
the utility function you have added suit their needs, or

2) Revert this patch, and propose a new patch that actually updates
the callers which need the somewhat strange semantics you are
proposing for getPointerTypeSizeInBits along with that function's
implementation.

That way we can review the utility routine and understand the use
cases for it. Currently, I have to assume that the callers you have
updated in this patch are either the only users or at least
representative of the common usage pattern.