[PATCH] D16275: [opaque pointer types] [NFC] GEP: replace get(Pointer)ElementType uses with get{Source, Result}ElementType.

[Manuel Jacob via llvm-commits]

mjacob added inline comments.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4090
@@ -4089,3 +4089,3 @@
 const SCEV *ScalarEvolution::createNodeForGEP(GEPOperator *GEP) {
   Value *Base = GEP->getOperand(0);
   // Don't attempt to analyze GEPs over unsized objects.
----------------
You could move this variable closer to its only consumer or even inline it.

================
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:517
@@ +516,3 @@
+      if (Ty->isPointerTy()) {
+        Ty = cast<GEPOperator>(I)->getSourceElementType();
+      } else {
----------------
Please add a comment why this is correct here, i.e. that `Ty` can be a pointer type only on the first iteration.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3022
@@ +3021,3 @@
+      if (Ty->isPointerTy()) {
+        Ty = cast<GEPOperator>(&I)->getSourceElementType();
+      } else {
----------------
Same here.

================
Comment at: lib/IR/ConstantFold.cpp:2045
@@ -2044,3 +2045,1 @@
-    Type *Ty = GetElementPtrInst::getIndexedType(
-        cast<PointerType>(Ptr->getScalarType())->getElementType(), Idxs);
     assert(Ty && "Invalid indices for GEP!");
----------------
Maybe this code was wrong, because `Ptr->getScalarType()` returns `Ptr` in any case. However I couldn't find a way to trigger a bug. I'll investigate this. Your changes look OK.

================
Comment at: lib/IR/ConstantsContext.h:228
@@ -227,2 +227,3 @@
   Type *SrcElementTy;
+  Type *ResElementTy;
   void anchor() override;
----------------
You should mention in the commit message that you add a field to this class.

================
Comment at: lib/Target/AArch64/AArch64FastISel.cpp:4829
@@ +4828,3 @@
+      if (Ty->isPointerTy()) {
+        Ty = cast<GEPOperator>(I)->getSourceElementType();
+      } else {
----------------
Please add a comment - same as above.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:697-698
@@ -696,6 +696,4 @@
   SmallVector<Value *, 4> Ops(GEPI->idx_begin(), GEPI->idx_begin() + Idx);
-  Type *AllocTy = GetElementPtrInst::getIndexedType(
-      cast<PointerType>(GEPI->getOperand(0)->getType()->getScalarType())
-          ->getElementType(),
-      Ops);
+  Type *AllocTy =
+    GetElementPtrInst::getIndexedType(GEPI->getSourceElementType(), Ops);
   if (!AllocTy || !AllocTy->isSized())
----------------
Can we use `getResultElementType()` here?

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1421
@@ -1421,3 +1420,3 @@
             // static for struct slots
-            if (J > 1 && CurTy->isStructTy())
+            if (J > 1 && CurTy && CurTy->isStructTy())
               return nullptr;
----------------
I think `CurTy` is implied by `J > 1`. IMHO the extra check creates confusion.


http://reviews.llvm.org/D16275