Browse Source

Fix a potential bug in dynamic structbuf indexing (#1754)

This was found by code inspection. When GEPs containing dynamic indices are translated into structbuf offsets, the logic for computing field offsets did not take data layout into account. This might however be dead code since in all of our tests plus a few custom ones I wrote, nested GEPs where not merged, so the GEP for accessing struct fields only contained constant indices (because the field name is known at compile-time), hence we would never go down that branch. But better safe than sorry.

Also consolidated some duplicate code for dealing with sequence types.
Tristan Labelle 6 years ago
parent
commit
49a9672d0f
1 changed files with 4 additions and 24 deletions
  1. 4 24
      lib/HLSL/HLOperationLower.cpp

+ 4 - 24
lib/HLSL/HLOperationLower.cpp

@@ -5909,8 +5909,8 @@ Value *GEPIdxToOffset(GetElementPtrInst *GEP, IRBuilder<> &Builder,
           continue;
           continue;
         }
         }
       }
       }
-      if (GEPIt->isPointerTy()) {
-        unsigned size = DL.getTypeAllocSize(GEPIt->getPointerElementType());
+      if (GEPIt->isPointerTy() || GEPIt->isArrayTy() || GEPIt->isVectorTy()) {
+        unsigned size = DL.getTypeAllocSize(GEPIt->getSequentialElementType());
         if (immIdx) {
         if (immIdx) {
           unsigned tempOffset = size * immIdx;
           unsigned tempOffset = size * immIdx;
           offset = Builder.CreateAdd(offset, OP->GetU32Const(tempOffset));
           offset = Builder.CreateAdd(offset, OP->GetU32Const(tempOffset));
@@ -5919,29 +5919,9 @@ Value *GEPIdxToOffset(GetElementPtrInst *GEP, IRBuilder<> &Builder,
           offset = Builder.CreateAdd(offset, tempOffset);
           offset = Builder.CreateAdd(offset, tempOffset);
         }
         }
       } else if (GEPIt->isStructTy()) {
       } else if (GEPIt->isStructTy()) {
-        unsigned structOffset = 0;
-        for (unsigned i = 0; i < immIdx; i++) {
-          structOffset += DL.getTypeAllocSize(GEPIt->getStructElementType(i));
-        }
+        const StructLayout *Layout = DL.getStructLayout(cast<StructType>(*GEPIt));
+        unsigned structOffset = Layout->getElementOffset(immIdx);
         offset = Builder.CreateAdd(offset, OP->GetU32Const(structOffset));
         offset = Builder.CreateAdd(offset, OP->GetU32Const(structOffset));
-      } else if (GEPIt->isArrayTy()) {
-        unsigned size = DL.getTypeAllocSize(GEPIt->getArrayElementType());
-        if (immIdx) {
-          unsigned tempOffset = size * immIdx;
-          offset = Builder.CreateAdd(offset, OP->GetU32Const(tempOffset));
-        } else {
-          Value *tempOffset = Builder.CreateMul(idx, OP->GetU32Const(size));
-          offset = Builder.CreateAdd(offset, tempOffset);
-        }
-      } else if (GEPIt->isVectorTy()) {
-        unsigned size = DL.getTypeAllocSize(GEPIt->getVectorElementType());
-        if (immIdx) {
-          unsigned tempOffset = size * immIdx;
-          offset = Builder.CreateAdd(offset, OP->GetU32Const(tempOffset));
-        } else {
-          Value *tempOffset = Builder.CreateMul(idx, OP->GetU32Const(size));
-          offset = Builder.CreateAdd(offset, tempOffset);
-        }
       } else {
       } else {
         gep_type_iterator temp = GEPIt;
         gep_type_iterator temp = GEPIt;
         temp++;
         temp++;