[llvm] r204851 - Fix problem with r204836
Eli Bendersky
eliben at google.com
Wed Mar 26 14:05:12 PDT 2014
On Wed, Mar 26, 2014 at 2:02 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 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"?
>
I guess they could, but as I mentioned earlier right now there's a
convenient parallel between arg_operands and getArgOperand, which are two
ways to get to the same thing. Calling it "args" would make this parallel
less clear: args() and getArgOperand() -- so why not getArg() then? I
suppose that whichever paint we pick for this bikeshed, it should be
consistent with the existing idioms. Don't you think?
Eli
>
> > 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140326/49edb741/attachment.html>
More information about the llvm-commits
mailing list