GEP index size can be specified in the DataLayout, introduced in D42123. However, there were still places
in which getIndexSizeInBits was used interchangeably with getPointerSizeInBits. This notably caused issues
with Instcombine's visitPtrToInt; but the unit tests was incorrect, so this remained undiscovered.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/Transforms/InstCombine/icmp-custom-dl.ll | ||
---|---|---|
11–12 | This change does not work for us. |
llvm/test/Transforms/InstCombine/icmp-custom-dl.ll | ||
---|---|---|
11–12 | I don't believe that size of GEP index and type for "ptrtoint" transformation are the same thing. At least, that's how I interpreted langref: 'The ‘ptrtoint’ instruction converts value to integer type ty2 by interpreting the pointer value as an integer'. InstCombine was transforming 'ptrtoint i8* to i64' to 'ptrtoint i8* to i32', followed by a zext. Here, where the pointer size is 40 bits, that would mean 8 bits have been chopped off. If you do not specify the index type, this change is NFC because index size defaults to pointer size. |
llvm/test/Transforms/InstCombine/icmp-custom-dl.ll | ||
---|---|---|
11–12 | I agree that we loose 8 bits in this case. Our promblem is in i40 which is not an integer and we do not have any arithmetic ISA for i40. |
llvm/test/Transforms/InstCombine/icmp-custom-dl.ll | ||
---|---|---|
11–12 | I am not entirely sure what the transformation that's happening in this test is supposed to show (I would be very happy if we required explanatory comments in tests!), but if the pointer range and the pointer size are not the same then I think any transformations involving ptrtoint / inttoptr are unsound. The only information that this gives us is that a pointer is not just an integer address. It may be an integer address that is sign / zero extended, an integer address that has some metadata in the high bits, a fat pointer, and so on. |
llvm/test/Transforms/InstCombine/icmp-custom-dl.ll | ||
---|---|---|
11–12 | I think this test was originally written to check that Instcombine is behaving correctly when specifying pointer index range. I.e (in)correctly transforming a ptrtoint instruction to an integer size of that of the pointer index size. (Though, with this change, it would be the pointer size instead)
I'm sorry, I don't quite understand this. Why would they be unsound? Pointer index range surely should have nothing to do with converting pointer values to/from integers. |
Is this missing test coverage?
Since it only requires datalayout, i wouldn't think it's impossible to test this.
Hi Joseph,
Sorry for the delay. I am currently checking your changes, I need a few more days to complete the investigation.
Regards,
Igor
After further review and testing, I have found that changing the effective SCEV type to be the pointer index type does not work. Though neither does it work when assumed to be pointer size. In many places, these are assumed to be the same. I'm unsure of which way to go with this. Do I force effective SCEV type to be the pointer size and enforce this, or change it to be index size and enforce this?
In my mind, SCEV would treat the evolution of a pointer to be that of it's offset, so it should bear the same type.
Does anyone have an opinion on this?
Hi Joseph,
Thank you very much for looking into this
I think effective SCEV type must be index size and it should be enforced. Without it, indeed many tests fail. Any pointer arithmetic should fit into index size, in other case target may not be able to handle it.
Something like
--- a/lib/Analysis/ScalarEvolution.cpp +++ b/lib/Analysis/ScalarEvolution.cpp @@ -5692,6 +5692,8 @@ ScalarEvolution::getRangeRef(const SCEV *S, if (SignHint == ScalarEvolution::HINT_RANGE_UNSIGNED) { // For a SCEVUnknown, ask ValueTracking. KnownBits Known = computeKnownBits(U->getValue(), DL, 0, &AC, nullptr, &DT); + if (Known.getBitWidth() > BitWidth) + Known = Known.trunc(BitWidth); if (Known.One != ~Known.Zero + 1)
I think you also missed
@@ -9261,7 +9261,7 @@ unsigned SelectionDAG::InferPtrAlignment(SDValue Ptr) const { const GlobalValue *GV; int64_t GVOffset = 0; if (TLI->isGAPlusOffset(Ptr.getNode(), GV, GVOffset)) { - unsigned IdxWidth = getDataLayout().getIndexTypeSizeInBits(GV->getType()); + unsigned IdxWidth = getDataLayout().getPointerTypeSizeInBits(GV->getType()); KnownBits Known(IdxWidth); llvm::computeKnownBits(GV, Known, getDataLayout());
With changes above most of the tests pass, but a few still failing . For example Analysis/ScalarEvolution/implied-via-addition.ll with target datalayout="p:40:64:64:32"
Currently we keep Pointer SCEV type as Index type because we don't have any special SCEV Expression type for pointers.
I suppose, we should add scAddPtrExpr (add index to pointer) and scPtrDiffExpr (pointers diff) to solve this problem. The scAddPtrExpr will always return PonterSizeType and will be expanded to GEP, not to "add". The scPtrDiffExpr will return IndexSizeType and will be expanded to trunc+sub.
Updated diff:
I am quite new to SCEV, but it really looks like it just shouldn't work on pointers sometimes. I've fixed it such that it works with custom dl on all tests, so hopefully this is okay.
Also fixed a few more places in which idx and ptr size are assumed to be the same.
Added tests which touch as many places I could manage where I changed ptr type to idx type. Many of the failures were caused due to the ptrtoint.
I think you also missed
@@ -9261,7 +9261,7 @@ unsigned SelectionDAG::InferPtrAlignment(SDValue Ptr) const { const GlobalValue *GV; int64_t GVOffset = 0; if (TLI->isGAPlusOffset(Ptr.getNode(), GV, GVOffset)) { - unsigned IdxWidth = getDataLayout().getIndexTypeSizeInBits(GV->getType()); + unsigned IdxWidth = getDataLayout().getPointerTypeSizeInBits(GV->getType()); KnownBits Known(IdxWidth); llvm::computeKnownBits(GV, Known, getDataLayout());
Surely this should stay as IndexType?
Without the change it fail in computeKnownBits on assert(ExpectedWidth == BitWidth && "V and Known should have same BitWidth");
And we should check pointer alignment.
I think emitPointerArithmetic (clang) should be updated also
diff --git a/lib/CodeGen/CGExprScalar.cpp b/lib/CodeGen/CGExprScalar.cpp index 105e937ca5b..01eb1a1f76e 100644 --- a/lib/CodeGen/CGExprScalar.cpp +++ b/lib/CodeGen/CGExprScalar.cpp @@ -3194,10 +3194,10 @@ static Value *emitPointerArithmetic(CodeGenFunction &CGF, expr->getRHS())) return CGF.Builder.CreateIntToPtr(index, pointer->getType()); - if (width != DL.getTypeSizeInBits(PtrTy)) { + if (width != DL.getIndexTypeSizeInBits(PtrTy)) { // Zero-extend or sign-extend the pointer value according to // whether the index is signed or not. - index = CGF.Builder.CreateIntCast(index, DL.getIntPtrType(PtrTy), isSigned, + index = CGF.Builder.CreateIntCast(index, DL.getIndexType(PtrTy), isSigned, "idx.ext"); }
Thanks
llvm/lib/Analysis/ScalarEvolution.cpp | ||
---|---|---|
5720 | Your patch will work when Index type < Pointer type. So it's ok for us. if (Known.getBitWidth() > BitWidth && truncatesEffectiveSCEVType(U->getValue()->getType()) Known = Known.trunc(BitWidth); And I still assume that new SCEV types scAddPtr and scPtrDiff will allow to remove these compensations. |
Brilliant. Thank you.
I don't have commit access. Can you commit this on my behalf please? Or I can try and request commit access.
Reverted in f798eb21eca97dc44ed40da52ece22780fb74230 as it was causing failures in http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-debian/builds/443/steps/test-check-all/logs/stdio and other bots.
Your patch will work when Index type < Pointer type. So it's ok for us.
In general, I'd add a function to ScalarEvoultion bool truncatesEffectiveSCEVType(Type *, int *NumOfTruncatedBits) that returns true if SCEV effective type smaller than real type and use it here:
if (Known.getBitWidth() > BitWidth && truncatesEffectiveSCEVType(U->getValue()->getType())
And I still assume that new SCEV types scAddPtr and scPtrDiff will allow to remove these compensations.