[PATCH] D75660: Remove CompositeType class

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for working on this @efriedma!



================
Comment at: llvm/include/llvm/IR/Instructions.h:1021
 
+  static Type *getIndexedTypeStep(Type *Agg, Value *Idx);
+  static Type *getIndexedTypeStep(Type *Agg, uint64_t Idx);
----------------
Please add some comments/documentation for these methods.


================
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)) {
----------------
Is there a reason you didn't just call this `getTypeAtIndex` ?


================
Comment at: llvm/lib/IR/Instructions.cpp:1621
+  if (auto Sequential = dyn_cast<SequentialType>(Ty)) {
+    if (!Idx->getType()->isIntOrIntVectorTy())
+      return nullptr;
----------------
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```
?


================
Comment at: llvm/lib/IR/Instructions.cpp:1643
+    return Ty;
+  for (IndexTy V : IdxList.slice(1)) {
+    Ty = GetElementPtrInst::getIndexedTypeStep(Ty, V);
----------------
nit: can this use `const IndexTy &`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75660