[llvm] r204851 - Fix problem with r204836
David Blaikie
dblaikie at gmail.com
Wed Mar 26 14:18:01 PDT 2014
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
> 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