Instead, expose whether the current type is an array or a struct, if an array
what the upper bound is, and if a struct the struct type itself. This is
in preparation for a later change which will make PointerType derive from
Type rather than SequentialType.
Details
Diff Detail
- Build Status
Buildable 1206 Build 1206: arc lint + arc unit
Event Timeline
I'm not quite following why this change is necessary - could you provide more detail?
What would happen/what doesn't work if we try to have generic_gep_type_iterator::operator* keep returning the Type* of the current type?
llvm/include/llvm/IR/GetElementPtrTypeIterator.h | ||
---|---|---|
57–58 | presumably this'd be a bit better as a dyn_cast? |
I think we could in principle continue to provide a way to access the "current type", but it would make the gep_type_iterator implementation even harder to follow once we move PointerType (for example, we would probably have to add type switches in the two places where we have CompositeType::getTypeAtIndex now). gep_type_iterator was already getting rather complicated as a result of supporting operator* (e.g. it takes an address space) and I would prefer not to add to that.
Seems odd to have an iterator-like thing without any op*, though.
Should we make op* return 'getIndexedType' (I realize this is a subtle API break to change the semantics of an existing operation - perhaps better to leave that as a cleanup a version or two from now (leave a FIXME)?)
I think you've stared at this code a fair bit more than I have (certainly more than I have recently - I'm probably the one to blame for the AddressSpace parameter being plumbed through a year ago or something) - could you summarize/explain why the need for such a variety of member functions on the iterator now? (getArray, isArray, isBoundedArray, getArrayBound, isStruct, getStruct, getStructOrNull)
It'd be nice if there were some more unifying API - currently this diversity seems a bit awkward.
Yes, it was the semantics break I was concerned about. Good idea to add the FIXME, will do.
I think you've stared at this code a fair bit more than I have (certainly more than I have recently - I'm probably the one to blame for the AddressSpace parameter being plumbed through a year ago or something) - could you summarize/explain why the need for such a variety of member functions on the iterator now? (getArray, isArray, isBoundedArray, getArrayBound, isStruct, getStruct, getStructOrNull)
It'd be nice if there were some more unifying API - currently this diversity seems a bit awkward.
Basically these are convenience functions around a small set of internal states (struct, unbounded array, bounded array) to help with readability at call sites.
In an earlier version of the patch [1] I had just two APIs: getStructType (which would return null for arrays) and getArrayBound (which could return a sentinel value meaning "unbounded"). Using those two functions, you can figure out which state you are in and effectively get the rest of the API, but that came at the cost of some readability (e.g. you would have to remember that "!getStructType" means "this is an array"). On balance it seemed better to have more API in gep_type_iterator.
Relatedly, I found that we only care about the array "boundedness" in relatively few places, which suggests to me that maybe we should look more closely at whether it is really necessary to expose that property and see if we can remove some of the API surface here. However, that seems like an orthogonal change to me.
[1] https://github.com/llvm-project/llvm-project/compare/master...pcc:pointertype
I'd probably be inclined to add some comments about how this API's a bit weird and could be more offset-focussed (including the gotchas/places where that doesn't quite hold (you were saying about the outer structness being interesting - though I couldn't quite follow how/why)) in a non-doc comment at the top of the class.
But if not, people might still find this discussion in the code review. Thanks for your patience.
llvm/trunk/include/llvm/Analysis/TargetTransformInfoImpl.h | ||
---|---|---|
516 ↗ | (On Diff #80016) | Yes please, sorry about the conflict. This would appear to be the right code change for D27357: diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h index 2934e9c..7bc6415 100644 --- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h +++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h @@ -483,7 +483,9 @@ public: int64_t Scale = 0; auto GTI = gep_type_begin(PointeeType, Operands); + Type *TargetType; for (auto I = Operands.begin(); I != Operands.end(); ++I, ++GTI) { + TargetType = GTI.getIndexedType(); // We assume that the cost of Scalar GEP with constant index and the // cost of Vector GEP with splat constant index are the same. const ConstantInt *ConstIdx = dyn_cast<ConstantInt>(*I); @@ -513,9 +515,8 @@ public: unsigned AS = (Ptr == nullptr ? 0 : Ptr->getType()->getPointerAddressSpace()); if (static_cast<T *>(this)->isLegalAddressingMode( - PointerType::get(Type::getInt8Ty(PointeeType->getContext()), AS), - const_cast<GlobalValue *>(BaseGV), - BaseOffset, HasBaseReg, Scale, AS)) { + TargetType, const_cast<GlobalValue *>(BaseGV), BaseOffset, + HasBaseReg, Scale, AS)) { return TTI::TCC_Free; } return TTI::TCC_Basic; |
llvm/trunk/include/llvm/Analysis/TargetTransformInfoImpl.h | ||
---|---|---|
516 ↗ | (On Diff #80016) | Thank you. I will use your code. |
presumably this'd be a bit better as a dyn_cast?