[llvm] r204851 - Fix problem with r204836
David Blaikie
dblaikie at gmail.com
Wed Mar 26 14:21:31 PDT 2014
On Wed, Mar 26, 2014 at 2:20 PM, Eli Bendersky <eliben at google.com> wrote:
>
>
>
> 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.
Ah, it just doesn't have bounds checking in getArgOperand - fair enough then.
If no one else speaks up, I guess status quo will hold - if the old
name was good enough, so's the new one. Sorry for the noise.
- David
>
> 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
>> >> >
>> >> >
>> >
>> >
>
>
More information about the llvm-commits
mailing list