[llvm] r204851 - Fix problem with r204836

[David Blaikie]

On Wed, Mar 26, 2014 at 1:59 PM, Eli Bendersky <eliben at google.com> wrote:
>
>
>
> On Wed, Mar 26, 2014 at 1:55 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Wed, Mar 26, 2014 at 1:41 PM, Eli Bendersky <eliben at google.com> wrote:
>> > Author: eliben
>> > Date: Wed Mar 26 15:41:15 2014
>> > New Revision: 204851
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=204851&view=rev
>> > Log:
>> > Fix problem with r204836
>> >
>> > In CallInst, op_end() points at the callee, which we don't want to
>> > iterate over
>> > when just iterating over arguments. Now take this into account when
>> > returning
>> > a iterator_range from arg_operands. Similar reasoning for InvokeInst.
>>
>> Having operands() differ from op_begin()/op_end() seems just a tad
>> subtle...
>>
>> Could we rename the existing range (op_begin/op_end) to something more
>> distinct (or rename the new one to something more distinct) to
>> separate these two concepts more clearly?
>
>
> Note that the range adapter is arg_operands(), rather than operands().

Could it just be called "args" or "arguments"?

> 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().
>
> 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.

Oh, that makes sense - they're the operands of the user.

>
> Eli
>
>
>
>>
>>
>> >
>> > Also adds a unit test to verify this actually works as expected.
>> >
>> >
>> > Modified:
>> >     llvm/trunk/include/llvm/IR/Instructions.h
>> >     llvm/trunk/unittests/IR/InstructionsTest.cpp
>> >
>> > Modified: llvm/trunk/include/llvm/IR/Instructions.h
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instructions.h?rev=204851&r1=204850&r2=204851&view=diff
>> >
>> > ==============================================================================
>> > --- llvm/trunk/include/llvm/IR/Instructions.h (original)
>> > +++ llvm/trunk/include/llvm/IR/Instructions.h Wed Mar 26 15:41:15 2014
>> > @@ -1297,12 +1297,14 @@ public:
>> >
>> >    /// arg_operands - iteration adapter for range-for loops.
>> >    iterator_range<op_iterator> arg_operands() {
>> > -    return iterator_range<op_iterator>(op_begin(), op_end());
>> > +    // The last operand in the op list is the callee - it's not one of
>> > the args
>> > +    // so we don't want to iterate over it.
>> > +    return iterator_range<op_iterator>(op_begin(), op_end() - 1);
>> >    }
>> >
>> >    /// arg_operands - iteration adapter for range-for loops.
>> >    iterator_range<const_op_iterator> arg_operands() const {
>> > -    return iterator_range<const_op_iterator>(op_begin(), op_end());
>> > +    return iterator_range<const_op_iterator>(op_begin(), op_end() - 1);
>> >    }
>> >
>> >    /// \brief Wrappers for getting the \c Use of a call argument.
>> > @@ -2954,12 +2956,12 @@ public:
>> >
>> >    /// arg_operands - iteration adapter for range-for loops.
>> >    iterator_range<op_iterator> arg_operands() {
>> > -    return iterator_range<op_iterator>(op_begin(), op_end());
>> > +    return iterator_range<op_iterator>(op_begin(), op_end() - 3);
>> >    }
>> >
>> >    /// arg_operands - iteration adapter for range-for loops.
>> >    iterator_range<const_op_iterator> arg_operands() const {
>> > -    return iterator_range<const_op_iterator>(op_begin(), op_end());
>> > +    return iterator_range<const_op_iterator>(op_begin(), op_end() - 3);
>> >    }
>> >
>> >    /// \brief Wrappers for getting the \c Use of a invoke argument.
>> >
>> > Modified: llvm/trunk/unittests/IR/InstructionsTest.cpp
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/InstructionsTest.cpp?rev=204851&r1=204850&r2=204851&view=diff
>> >
>> > ==============================================================================
>> > --- llvm/trunk/unittests/IR/InstructionsTest.cpp (original)
>> > +++ llvm/trunk/unittests/IR/InstructionsTest.cpp Wed Mar 26 15:41:15
>> > 2014
>> > @@ -14,11 +14,14 @@
>> >  #include "llvm/IR/Constants.h"
>> >  #include "llvm/IR/DataLayout.h"
>> >  #include "llvm/IR/DerivedTypes.h"
>> > +#include "llvm/IR/Function.h"
>> >  #include "llvm/IR/IRBuilder.h"
>> >  #include "llvm/IR/LLVMContext.h"
>> >  #include "llvm/IR/MDBuilder.h"
>> > +#include "llvm/IR/Module.h"
>> >  #include "llvm/IR/Operator.h"
>> >  #include "gtest/gtest.h"
>> > +#include <memory>
>> >
>> >  namespace llvm {
>> >  namespace {
>> > @@ -47,6 +50,29 @@ TEST(InstructionsTest, ReturnInst) {
>> >    delete r1;
>> >  }
>> >
>> > +TEST(InstructionsTest, CallInst) {
>> > +  LLVMContext &C(getGlobalContext());
>> > +  std::unique_ptr<Module> M(new Module("MyModule", C));
>> > +
>> > +  Type *ArgTypes[] = {Type::getInt8Ty(C), Type::getInt32Ty(C),
>> > +                      Type::getInt64Ty(C)};
>> > +  FunctionType *FTy = FunctionType::get(Type::getVoidTy(C), ArgTypes,
>> > false);
>> > +  Function *F = Function::Create(FTy, Function::ExternalLinkage, "",
>> > M.get());
>> > +
>> > +  Value *Args[] = {ConstantInt::get(Type::getInt8Ty(C), 20),
>> > +                   ConstantInt::get(Type::getInt32Ty(C), 9999),
>> > +                   ConstantInt::get(Type::getInt64Ty(C), 42)};
>> > +  CallInst *Call = CallInst::Create(F, Args);
>> > +
>> > +  // Make sure iteration over a call's arguments works as expected.
>> > +  unsigned Idx = 0;
>> > +  for (Value *Arg : Call->arg_operands()) {
>> > +    EXPECT_EQ(ArgTypes[Idx], Arg->getType());
>> > +    EXPECT_EQ(Call->getArgOperand(Idx)->getType(), Arg->getType());
>> > +    Idx++;
>> > +  }
>> > +}
>> > +
>> >  TEST(InstructionsTest, BranchInst) {
>> >    LLVMContext &C(getGlobalContext());
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>