[llvm] r204851 - Fix problem with r204836
Eli Bendersky
eliben at google.com
Wed Mar 26 14:20:08 PDT 2014
On Wed, Mar 26, 2014 at 2:18 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Wed, Mar 26, 2014 at 2:05 PM, Eli Bendersky <eliben at google.com> wrote:
> >
> >
> >
> > 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.
>
> Not exactly - getArgOperand is just an alias for getOperand - it
> doesn't have the same "all operands except the last one" that you're
> adding to arg_operands()'s behavior
>
Oh, but it does:
unsigned getNumArgOperands() const { return getNumOperands() - 1; }
Since getArgOperand is a "C-like" accessor that runs from 0 to
getNumArgOperands [exclusive], this is equivalent to the behavior pf
arg_operands.
Eli
>
> > 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?
>
> Given the difference in behavior (getArgOperand can retrieve the
> callee but arg_operands cannot) I'm not entirely sure they should be
> the same. Though perhaps getArgOperand is meant to have the same
> behavior and it just should have an assert in there, then the names
> should be the same (& what that name is is still up for debate - I'm
> not sure that "getOperand" and "getArgOperand" are necessarily clear -
> but I don't work in the IR optimizers much so don't have a feel for
> appropriate/meaningful naming & happy to defer to those who do)
>
> - David
>
> >
> > 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/7936f096/attachment.html>
More information about the llvm-commits
mailing list