<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Mar 26, 2014 at 1:55 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Wed, Mar 26, 2014 at 1:41 PM, Eli Bendersky <<a href="mailto:eliben@google.com">eliben@google.com</a>> 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 iterate over<br>
> when just iterating over arguments. Now take this into account when returning<br>
> a iterator_range from arg_operands. Similar reasoning for InvokeInst.<br>
<br>
</div>Having operands() differ from op_begin()/op_end() seems just a tad 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></blockquote><div><br></div><div>Note that the range adapter is arg_operands(), rather than operands(). This distinction, while subtle, is important because the existing accessors are also called getArgOperand. So there already is a distinction between "operands" (generic operand traits defined by OperandTraits and including *all* the operands, like the callee in CallInst, for example) and "arg operands". The new adapters I added are called arg_operands() for this reason, and not operands(). </div>
<div><br></div><div>I don't think we can rename op_begin/op_end - these come through macros from OperandTraits.h and too much within LLVM depends on them.</div><div><br></div><div>Eli</div><div><br></div><div><br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><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: <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>
> --- llvm/trunk/include/llvm/IR/Instructions.h (original)<br>
> +++ llvm/trunk/include/llvm/IR/Instructions.h Wed Mar 26 15:41:15 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 of 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() - 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() - 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: <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>
> --- llvm/trunk/unittests/IR/InstructionsTest.cpp (original)<br>
> +++ llvm/trunk/unittests/IR/InstructionsTest.cpp Wed Mar 26 15:41:15 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), ArgTypes, false);<br>
> + Function *F = Function::Create(FTy, Function::ExternalLinkage, "", 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>
</div></div></blockquote></div><br></div></div>