GEP::getIndexValid() with other iterators

Hi all,

currently, GetElementPtrInst has a method getIndexedType, which has a
templated variant. You pass in a begin and an end iterator, and it will find
the indexed type for those. However, both iterators must iterate over Value*.

For some argpromotion code, I would like to pass in iterators that iterate
over unsigneds instead of Value*. I currently solve this by transforming my
vector<unsigned> into a vector<Value*>, but this is hardly an efficient
solution.

Currently, however, there is already a templated version of getIndexedType,
probably so it can iterate over any pointer. In there, the begin pointer is
cast as follows:
(const Value*)&*IdxBegin

Though this works, this has the nasty side effect of accepting a lot of
iterator types that are not really supported. In particular, when I passed in
an iterator over unsigned, the compiler would happily cast the resulting
unsigned* to a const Value*, resulting in all kinds of badness at runtime (and
not even a warning at compile time).

However, simply removing this cast seems to work for me, so I suppose there is
some compiler out there that will warn about this? Or is the cast really not
needed (anymore)?

Anyway, when one removes this cast and also makes the other getIndexedType
implementation (which is called by this one) a template, passing in an
iterator over unsigned should also work. The actual types over which an
iterator is allowed should now be limited by the parameters to
CompositeType::indexValid() and CompositeType::getTypeAtIndex() (which
currently are Value* or unsigned).

I've attached a patch which does exactly this. Since my template-fu is not so
strong, please review :slight_smile: I've also attached another patch to argpromotion,
which passes an unsigned iterator to getIndexedType()
(look for the comment
  // This uses the new templated getIndexdType
in the code)

Gr.

Matthijs

argpromotion.diff (24.6 KB)

getindexedtype.diff (3.74 KB)

Hi all,

once more with the patch inline for easy review. I did not include the
argpromotion pass here, since it's not the main topic of this post.

Gr.

Matthijs

Index: lib/VMCore/Instructions.cpp

Hi all,

no comments on this patch? I haven't seen any problems so far, so I'll be
committing this tomorrow.

Gr.

Matthijs

Hi Matthijs,

I'd prefer to not turn this into a template. Why not just define a version that takes an array of uint64_t's or something like that?

-Chris

Hi Chris,

I'd prefer to not turn this into a template. Why not just define a
version that takes an array of uint64_t's or something like that?

because I want to be able to pass in iterators. I could define a version that
takes std<uint64_t>::iterators, but next thing we know, we also need them for
lists, SmallVectors, etc. That's why one of the original getIndexedType
methods is a template, and that's why I think it makes sense to make another
one a template.

Any particular objections to this? Is the code size increase a problem?
AFAICS, in cases where you need this method, it will be a tradeoff between
speed (having to iterate all your indices and create a new list with the
Value* versions) and code size (having a version of getIndexedPointer that can
handle your particular flavour of iterator).

Gr.

Matthijs

Hi Chris,

I'd prefer to not turn this into a template. Why not just define a
version that takes an array of uint64_t's or something like that?

because I want to be able to pass in iterators. I could define a version that
takes std<uint64_t>::iterators, but next thing we know, we also need them for
lists, SmallVectors, etc. That's why one of the original getIndexedType
methods is a template, and that's why I think it makes sense to make another
one a template.

What flavor of iterators do you want to pass in? vector or smallvector? If so, a pointer to the first element + extents is fine.

Any particular objections to this? Is the code size increase a problem?
AFAICS, in cases where you need this method, it will be a tradeoff between
speed (having to iterate all your indices and create a new list with the
Value* versions) and code size (having a version of getIndexedPointer that can
handle your particular flavour of iterator).

My basic objection is that I don't like tons of code in header files. It obfuscates the header and slows down compile times (of llvm itself)

-Chris

Hi Chris,

What flavor of iterators do you want to pass in? vector or
smallvector? If so, a pointer to the first element + extents is fine.

I was passing a std::vector in. And you are quite right, first element + size
works like a charm.

Since I am using a vector of uint64_t's instead of Value*, I did need to add
an extra version of getIndexedType(), taking a uint64_t* instead of Value**.

My basic objection is that I don't like tons of code in header files.
It obfuscates the header and slows down compile times (of llvm itself)

Ah, that is even a better reason :slight_smile:

I did use a template function to prevent duplicating code, but since it is
only used in the implementation of two getIndexedType() methods, it is in the
cpp file.

So, is this better?

Gr.

Matthijs

Index: lib/VMCore/Instructions.cpp

I did use a template function to prevent duplicating code, but since it is
only used in the implementation of two getIndexedType() methods, it is in the
cpp file.

So, is this better?

Looks great to me, please commit! Thanks Matthijs,

-Chris