<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Nov 29, 2016 at 10:20 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr" class="gmail-m_8741109653652247265gmail_msg">'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)<br class="gmail-m_8741109653652247265gmail_msg"></div></div></blockquote><div><br></div><div>Makes sense, will do. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr" class="gmail-m_8741109653652247265gmail_msg">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.<br></div></div></blockquote><div><br></div><div>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*).</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr" class="gmail-m_8741109653652247265gmail_msg">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.</div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>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:</div><div>- they pretty much only ever using the outer struct type to query the byte offset of the current element</div><div>- they hardly ever call getIndexedType() on struct types</div><div>- they pretty much only ever use the result of getIndexedType() to call getTypeAllocSize() on it in preparation for calculating a byte offset</div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Peter</div><div><br></div><div>[0] <a href="http://llvm-cs.pcc.me.uk/lib/Transforms/Utils/SimplifyCFG.cpp#1388">http://llvm-cs.pcc.me.uk/lib/Transforms/Utils/SimplifyCFG.cpp#1388</a></div><div>[1] <a href="http://llvm-cs.pcc.me.uk/lib/Analysis/BasicAliasAnalysis.cpp#424">http://llvm-cs.pcc.me.uk/lib/Analysis/BasicAliasAnalysis.cpp#424</a></div><div>[2] <a href="http://llvm-cs.pcc.me.uk/lib/Analysis/BasicAliasAnalysis.cpp#1041">http://llvm-cs.pcc.me.uk/lib/Analysis/BasicAliasAnalysis.cpp#1041</a></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div class="gmail-h5"><div dir="ltr" class="gmail-m_8741109653652247265gmail_msg"><br></div><div dir="ltr" class="gmail-m_8741109653652247265gmail_msg"><br></div><br class="gmail-m_8741109653652247265gmail_msg"><div class="gmail_quote gmail-m_8741109653652247265gmail_msg"><div dir="ltr" class="gmail-m_8741109653652247265gmail_msg">On Sat, Nov 26, 2016 at 10:11 PM Peter Collingbourne via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail-m_8741109653652247265gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail-m_8741109653652247265gmail_msg"></div><blockquote class="gmail_quote gmail-m_8741109653652247265gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">pcc updated this revision to Diff 79348.<br class="gmail-m_8741109653652247265gmail_msg">
pcc marked an inline comment as done.<br class="gmail-m_8741109653652247265gmail_msg">
pcc added a comment.<br class="gmail-m_8741109653652247265gmail_msg">
Herald added a subscriber: amehsan.<br class="gmail-m_8741109653652247265gmail_msg">
<br class="gmail-m_8741109653652247265gmail_msg">
- Formatting, address review comments, fix bug I introduced in NaryReassociate<br class="gmail-m_8741109653652247265gmail_msg">
<br class="gmail-m_8741109653652247265gmail_msg">
<br class="gmail-m_8741109653652247265gmail_msg">
<a href="https://reviews.llvm.org/D26594" rel="noreferrer" class="gmail-m_8741109653652247265gmail_msg" target="_blank">https://reviews.llvm.org/<wbr>D26594</a><br class="gmail-m_8741109653652247265gmail_msg">
<br class="gmail-m_8741109653652247265gmail_msg">
Files:<br class="gmail-m_8741109653652247265gmail_msg">
llvm/include/llvm/Analysis/<wbr>TargetTransformInfoImpl.h<br class="gmail-m_8741109653652247265gmail_msg">
llvm/include/llvm/IR/<wbr>GetElementPtrTypeIterator.h<br class="gmail-m_8741109653652247265gmail_msg">
llvm/include/llvm/Transforms/<wbr>Utils/Local.h<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Analysis/<wbr>BasicAliasAnalysis.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Analysis/InlineCost.<wbr>cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Analysis/<wbr>ValueTracking.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Analysis/VectorUtils.<wbr>cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/CodeGen/<wbr>CodeGenPrepare.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/CodeGen/SelectionDAG/<wbr>FastISel.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/CodeGen/SelectionDAG/<wbr>SelectionDAGBuilder.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/ExecutionEngine/<wbr>Interpreter/Execution.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/IR/ConstantFold.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/IR/Constants.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/IR/DataLayout.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/IR/Operator.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Target/AArch64/<wbr>AArch64FastISel.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Target/AArch64/<wbr>AArch64ISelLowering.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Target/ARM/<wbr>ARMFastISel.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Target/Mips/<wbr>MipsFastISel.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Target/PowerPC/<wbr>PPCFastISel.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Target/WebAssembly/<wbr>WebAssemblyFastISel.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Target/X86/<wbr>X86FastISel.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Transforms/IPO/<wbr>GlobalOpt.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Transforms/<wbr>InstCombine/<wbr>InstCombineCompares.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Transforms/<wbr>InstCombine/<wbr>InstructionCombining.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Transforms/Scalar/<wbr>MemCpyOptimizer.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Transforms/Scalar/<wbr>NaryReassociate.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Transforms/Scalar/<wbr>SROA.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Transforms/Scalar/<wbr>SeparateConstOffsetFromGEP.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Transforms/Scalar/<wbr>StraightLineStrengthReduce.cpp<br class="gmail-m_8741109653652247265gmail_msg">
llvm/lib/Transforms/Utils/<wbr>SimplifyCFG.cpp<br class="gmail-m_8741109653652247265gmail_msg">
<br class="gmail-m_8741109653652247265gmail_msg">
</blockquote></div></div></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</div></div>