[PATCH] D75660: Remove CompositeType class

[Eli Friedman via Phabricator via llvm-commits]

efriedma marked 3 inline comments as done.
efriedma added inline comments.


================
Comment at: llvm/lib/IR/Instructions.cpp:1615
 
-/// getIndexedType - Returns the type of the element that would be accessed with
-/// a gep instruction with the specified parameters.
-///
-/// The Idxs pointer should point to a continuous piece of memory containing the
-/// indices, either as Value* or uint64_t.
-///
-/// A null type is returned if the indices are invalid for the specified
-/// pointer type.
-///
-template <typename IndexTy>
-static Type *getIndexedTypeInternal(Type *Agg, ArrayRef<IndexTy> IdxList) {
-  // Handle the special case of the empty set index set, which is always valid.
-  if (IdxList.empty())
-    return Agg;
+Type *GetElementPtrInst::getIndexedTypeStep(Type *Ty, Value* Idx) {
+  if (auto Struct = dyn_cast<StructType>(Ty)) {
----------------
sdesmalen wrote:
> Is there a reason you didn't just call this `getTypeAtIndex` ?
I guess I could call it that; didn't really think much about the name.  (I originally thought of it as splitting out each "step" of getIndexedType into a separate method.)


================
Comment at: llvm/lib/IR/Instructions.cpp:1621
+  if (auto Sequential = dyn_cast<SequentialType>(Ty)) {
+    if (!Idx->getType()->isIntOrIntVectorTy())
+      return nullptr;
----------------
sdesmalen wrote:
> Maybe I'm misunderstanding this method, but checking that the indexed type is of type integer, does that exclude the case:
> ```getelementptr float, float* %ptr, i64 0```
> ?
The point of the isIntOrIntVectorTy() check is to exclude something silly like a float index into an array.  It has nothing to do with the type of the pointer.


================
Comment at: llvm/lib/IR/Instructions.cpp:1643
+    return Ty;
+  for (IndexTy V : IdxList.slice(1)) {
+    Ty = GetElementPtrInst::getIndexedTypeStep(Ty, V);
----------------
sdesmalen wrote:
> nit: can this use `const IndexTy &`?
Yes, but it wouldn't really matter; IndexTy is cheap to copy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75660/new/

https://reviews.llvm.org/D75660