[PATCH] D26594: IR: Change the gep_type_iterator API to avoid always exposing the "current" type.

Tue Nov 29 11:37:26 PST 2016

On Tue, Nov 29, 2016 at 10:20 AM, David Blaikie <dblaikie at gmail.com> wrote:

> 'isArray'/array bound returning true for vector types seems like it might
> be a bit confusing for some users. Should it use the sequential
> terminology? (would that suit/be correct)
>

Makes sense, will do.

It seems still really confusing overall (maybe I'm just not sufficiently
> familiar with the context) to have various APIs returning the outer
> (struct) type, some returning the inner (array member, etc) type, etc.
>

I agree but to a certain extent that's what we already have now (with
operator* and getIndexedType). I suppose I'd argue that I'm making things
less confusing by avoiding exposing "artificial" types (i.e. the
PointerTypes sometimes created by operator*).

Feels like there should be a more consistent API for this abstraction -
> even if it means a bit more work in some clients (or some (probably
> non-member)? helper functions). Do you have a good understanding (you could
> help impart to me) why this iterator simultaneously talks about the outer
> and the indexed type at any point? I think that's what's complicating this.
>

The outer type is part of the context that you need to interpret the
current index, i.e. you need to calculate the byte offset differently
depending on whether the outer type is an array or a struct. We also need
to know whether the outer type is a struct because we use that in some
places to decide whether/how we can legally modify that GEP index (e.g.
[0], and [1] which leads to [2]). The indexed type is needed to calculate
the byte offset in the case where the outer type is an array.

I think I agree with you that there ought to be a simpler API for this. A
few insights I had while looking at the users of this class:
- they pretty much only ever using the outer struct type to query the byte
offset of the current element
- they hardly ever call getIndexedType() on struct types
- they pretty much only ever use the result of getIndexedType() to call
getTypeAllocSize() on it in preparation for calculating a byte offset
In summary: pretty much all clients of this API are using it to calculate
byte offsets. That would lead me to suspect that our hypothetical better
API would somehow involve exposing not much more than the byte offset for
the current element, and a multiplier for arrays (to be able to handle
non-constant offsets correctly). I think we'd still need to expose "whether
the outer type is a struct" for the reasons I mentioned above.

That all feels like a change which could be incrementally made on top of
this one, though. i.e. fundamentally what this change is doing is removing
direct access to the "outer type"; subsequent changes would remove various
forms of "indirect" access.

Peter

[0] http://llvm-cs.pcc.me.uk/lib/Transforms/Utils/SimplifyCFG.cpp#1388
[1] http://llvm-cs.pcc.me.uk/lib/Analysis/BasicAliasAnalysis.cpp#424
[2] http://llvm-cs.pcc.me.uk/lib/Analysis/BasicAliasAnalysis.cpp#1041

>
>
> On Sat, Nov 26, 2016 at 10:11 PM Peter Collingbourne via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> pcc updated this revision to Diff 79348.
> pcc marked an inline comment as done.
> pcc added a comment.
> Herald added a subscriber: amehsan.
>
> - Formatting, address review comments, fix bug I introduced in
> NaryReassociate
>
>
> https://reviews.llvm.org/D26594
>
> Files:
>   llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
>   llvm/include/llvm/IR/GetElementPtrTypeIterator.h
>   llvm/include/llvm/Transforms/Utils/Local.h
>   llvm/lib/Analysis/BasicAliasAnalysis.cpp
>   llvm/lib/Analysis/InlineCost.cpp
>   llvm/lib/Analysis/ValueTracking.cpp
>   llvm/lib/Analysis/VectorUtils.cpp
>   llvm/lib/CodeGen/CodeGenPrepare.cpp
>   llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
>   llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>   llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
>   llvm/lib/IR/ConstantFold.cpp
>   llvm/lib/IR/Constants.cpp
>   llvm/lib/IR/DataLayout.cpp
>   llvm/lib/IR/Operator.cpp
>   llvm/lib/Target/AArch64/AArch64FastISel.cpp
>   llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
>   llvm/lib/Target/ARM/ARMFastISel.cpp
>   llvm/lib/Target/Mips/MipsFastISel.cpp
>   llvm/lib/Target/PowerPC/PPCFastISel.cpp
>   llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
>   llvm/lib/Target/X86/X86FastISel.cpp
>   llvm/lib/Transforms/IPO/GlobalOpt.cpp
>   llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
>   llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
>   llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>   llvm/lib/Transforms/Scalar/NaryReassociate.cpp
>   llvm/lib/Transforms/Scalar/SROA.cpp
>   llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
>   llvm/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp
>   llvm/lib/Transforms/Utils/SimplifyCFG.cpp
>
>

-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161129/d32a9b62/attachment.html>