<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Mar 26, 2014 at 2:18 PM, 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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Wed, Mar 26, 2014 at 2:05 PM, Eli Bendersky <<a href="mailto:eliben@google.com">eliben@google.com</a>> wrote:<br>


><br>
><br>
><br>
> On Wed, Mar 26, 2014 at 2:02 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Wed, Mar 26, 2014 at 1:59 PM, Eli Bendersky <<a href="mailto:eliben@google.com">eliben@google.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> ><br>
>> > On Wed, Mar 26, 2014 at 1:55 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Wed, Mar 26, 2014 at 1:41 PM, Eli Bendersky <<a href="mailto:eliben@google.com">eliben@google.com</a>><br>
>> >> wrote:<br>
>> >> > Author: eliben<br>
>> >> > Date: Wed Mar 26 15:41:15 2014<br>
>> >> > New Revision: 204851<br>
>> >> ><br>
>> >> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=204851&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=204851&view=rev</a><br>
>> >> > Log:<br>
>> >> > Fix problem with r204836<br>
>> >> ><br>
>> >> > In CallInst, op_end() points at the callee, which we don't want to<br>
>> >> > iterate over<br>
>> >> > when just iterating over arguments. Now take this into account when<br>
>> >> > returning<br>
>> >> > a iterator_range from arg_operands. Similar reasoning for InvokeInst.<br>
>> >><br>
>> >> Having operands() differ from op_begin()/op_end() seems just a tad<br>
>> >> subtle...<br>
>> >><br>
>> >> Could we rename the existing range (op_begin/op_end) to something more<br>
>> >> distinct (or rename the new one to something more distinct) to<br>
>> >> separate these two concepts more clearly?<br>
>> ><br>
>> ><br>
>> > Note that the range adapter is arg_operands(), rather than operands().<br>
>><br>
>> Could it just be called "args" or "arguments"?<br>
><br>
><br>
> I guess they could, but as I mentioned earlier right now there's a<br>
> convenient parallel between arg_operands and getArgOperand, which are two<br>
> ways to get to the same thing.<br>
<br>
</div></div>Not exactly - getArgOperand is just an alias for getOperand - it<br>
doesn't have the same "all operands except the last one" that you're<br>
adding to arg_operands()'s behavior<br></blockquote><div><br></div><div>Oh, but it does:</div><div><br></div><div><div>  unsigned getNumArgOperands() const { return getNumOperands() - 1; }</div></div><div><br></div><div>

Since getArgOperand is a "C-like" accessor that runs from 0 to getNumArgOperands [exclusive], this is equivalent to the behavior pf arg_operands.</div><div><br></div><div>Eli</div><div><br></div><div><br></div>
<div>
 </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><br>
> Calling it "args" would make this parallel<br>
> less clear: args() and getArgOperand() -- so why not getArg() then? I<br>
> suppose that whichever paint we pick for this bikeshed, it should be<br>
> consistent with the existing idioms. Don't you think?<br>
<br>
</div>Given the difference in behavior (getArgOperand can retrieve the<br>
callee but arg_operands cannot) I'm not entirely sure they should be<br>
the same. Though perhaps getArgOperand is meant to have the same<br>
behavior and it just should have an assert in there, then the names<br>
should be the same (& what that name is is still up for debate - I'm<br>
not sure that "getOperand" and "getArgOperand" are necessarily clear -<br>
but I don't work in the IR optimizers much so don't have a feel for<br>
appropriate/meaningful naming & happy to defer to those who do)<br>
<span class=""><font color="#888888"><br>
- David<br>
</font></span><div class=""><div class="h5"><br>
><br>
> Eli<br>
><br>
><br>
>><br>
>><br>
>> > This<br>
>> > distinction, while subtle, is important because the existing accessors<br>
>> > are<br>
>> > also called getArgOperand. So there already is a distinction between<br>
>> > "operands" (generic operand traits defined by OperandTraits and<br>
>> > including<br>
>> > *all* the operands, like the callee in CallInst, for example) and "arg<br>
>> > operands". The new adapters I added are called arg_operands() for this<br>
>> > reason, and not operands().<br>
>> ><br>
>> > I don't think we can rename op_begin/op_end - these come through macros<br>
>> > from<br>
>> > OperandTraits.h and too much within LLVM depends on them.<br>
>><br>
>> Oh, that makes sense - they're the operands of the user.<br>
>><br>
>> ><br>
>> > Eli<br>
>> ><br>
>> ><br>
>> ><br>
>> >><br>
>> >><br>
>> >> ><br>
>> >> > Also adds a unit test to verify this actually works as expected.<br>
>> >> ><br>
>> >> ><br>
>> >> > Modified:<br>
>> >> >     llvm/trunk/include/llvm/IR/Instructions.h<br>
>> >> >     llvm/trunk/unittests/IR/InstructionsTest.cpp<br>
>> >> ><br>
>> >> > Modified: llvm/trunk/include/llvm/IR/Instructions.h<br>
>> >> > URL:<br>
>> >> ><br>
>> >> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instructions.h?rev=204851&r1=204850&r2=204851&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instructions.h?rev=204851&r1=204850&r2=204851&view=diff</a><br>


>> >> ><br>
>> >> ><br>
>> >> > ==============================================================================<br>
>> >> > --- llvm/trunk/include/llvm/IR/Instructions.h (original)<br>
>> >> > +++ llvm/trunk/include/llvm/IR/Instructions.h Wed Mar 26 15:41:15<br>
>> >> > 2014<br>
>> >> > @@ -1297,12 +1297,14 @@ public:<br>
>> >> ><br>
>> >> >    /// arg_operands - iteration adapter for range-for loops.<br>
>> >> >    iterator_range<op_iterator> arg_operands() {<br>
>> >> > -    return iterator_range<op_iterator>(op_begin(), op_end());<br>
>> >> > +    // The last operand in the op list is the callee - it's not one<br>
>> >> > of<br>
>> >> > the args<br>
>> >> > +    // so we don't want to iterate over it.<br>
>> >> > +    return iterator_range<op_iterator>(op_begin(), op_end() - 1);<br>
>> >> >    }<br>
>> >> ><br>
>> >> >    /// arg_operands - iteration adapter for range-for loops.<br>
>> >> >    iterator_range<const_op_iterator> arg_operands() const {<br>
>> >> > -    return iterator_range<const_op_iterator>(op_begin(), op_end());<br>
>> >> > +    return iterator_range<const_op_iterator>(op_begin(), op_end() -<br>
>> >> > 1);<br>
>> >> >    }<br>
>> >> ><br>
>> >> >    /// \brief Wrappers for getting the \c Use of a call argument.<br>
>> >> > @@ -2954,12 +2956,12 @@ public:<br>
>> >> ><br>
>> >> >    /// arg_operands - iteration adapter for range-for loops.<br>
>> >> >    iterator_range<op_iterator> arg_operands() {<br>
>> >> > -    return iterator_range<op_iterator>(op_begin(), op_end());<br>
>> >> > +    return iterator_range<op_iterator>(op_begin(), op_end() - 3);<br>
>> >> >    }<br>
>> >> ><br>
>> >> >    /// arg_operands - iteration adapter for range-for loops.<br>
>> >> >    iterator_range<const_op_iterator> arg_operands() const {<br>
>> >> > -    return iterator_range<const_op_iterator>(op_begin(), op_end());<br>
>> >> > +    return iterator_range<const_op_iterator>(op_begin(), op_end() -<br>
>> >> > 3);<br>
>> >> >    }<br>
>> >> ><br>
>> >> >    /// \brief Wrappers for getting the \c Use of a invoke argument.<br>
>> >> ><br>
>> >> > Modified: llvm/trunk/unittests/IR/InstructionsTest.cpp<br>
>> >> > URL:<br>
>> >> ><br>
>> >> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/InstructionsTest.cpp?rev=204851&r1=204850&r2=204851&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/InstructionsTest.cpp?rev=204851&r1=204850&r2=204851&view=diff</a><br>


>> >> ><br>
>> >> ><br>
>> >> > ==============================================================================<br>
>> >> > --- llvm/trunk/unittests/IR/InstructionsTest.cpp (original)<br>
>> >> > +++ llvm/trunk/unittests/IR/InstructionsTest.cpp Wed Mar 26 15:41:15<br>
>> >> > 2014<br>
>> >> > @@ -14,11 +14,14 @@<br>
>> >> >  #include "llvm/IR/Constants.h"<br>
>> >> >  #include "llvm/IR/DataLayout.h"<br>
>> >> >  #include "llvm/IR/DerivedTypes.h"<br>
>> >> > +#include "llvm/IR/Function.h"<br>
>> >> >  #include "llvm/IR/IRBuilder.h"<br>
>> >> >  #include "llvm/IR/LLVMContext.h"<br>
>> >> >  #include "llvm/IR/MDBuilder.h"<br>
>> >> > +#include "llvm/IR/Module.h"<br>
>> >> >  #include "llvm/IR/Operator.h"<br>
>> >> >  #include "gtest/gtest.h"<br>
>> >> > +#include <memory><br>
>> >> ><br>
>> >> >  namespace llvm {<br>
>> >> >  namespace {<br>
>> >> > @@ -47,6 +50,29 @@ TEST(InstructionsTest, ReturnInst) {<br>
>> >> >    delete r1;<br>
>> >> >  }<br>
>> >> ><br>
>> >> > +TEST(InstructionsTest, CallInst) {<br>
>> >> > +  LLVMContext &C(getGlobalContext());<br>
>> >> > +  std::unique_ptr<Module> M(new Module("MyModule", C));<br>
>> >> > +<br>
>> >> > +  Type *ArgTypes[] = {Type::getInt8Ty(C), Type::getInt32Ty(C),<br>
>> >> > +                      Type::getInt64Ty(C)};<br>
>> >> > +  FunctionType *FTy = FunctionType::get(Type::getVoidTy(C),<br>
>> >> > ArgTypes,<br>
>> >> > false);<br>
>> >> > +  Function *F = Function::Create(FTy, Function::ExternalLinkage, "",<br>
>> >> > M.get());<br>
>> >> > +<br>
>> >> > +  Value *Args[] = {ConstantInt::get(Type::getInt8Ty(C), 20),<br>
>> >> > +                   ConstantInt::get(Type::getInt32Ty(C), 9999),<br>
>> >> > +                   ConstantInt::get(Type::getInt64Ty(C), 42)};<br>
>> >> > +  CallInst *Call = CallInst::Create(F, Args);<br>
>> >> > +<br>
>> >> > +  // Make sure iteration over a call's arguments works as expected.<br>
>> >> > +  unsigned Idx = 0;<br>
>> >> > +  for (Value *Arg : Call->arg_operands()) {<br>
>> >> > +    EXPECT_EQ(ArgTypes[Idx], Arg->getType());<br>
>> >> > +    EXPECT_EQ(Call->getArgOperand(Idx)->getType(), Arg->getType());<br>
>> >> > +    Idx++;<br>
>> >> > +  }<br>
>> >> > +}<br>
>> >> > +<br>
>> >> >  TEST(InstructionsTest, BranchInst) {<br>
>> >> >    LLVMContext &C(getGlobalContext());<br>
>> >> ><br>
>> >> ><br>
>> >> ><br>
>> >> > _______________________________________________<br>
>> >> > llvm-commits mailing list<br>
>> >> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> >> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>