Handle a gap while matching SIL patterns for hoisting array bound checks #65573
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -530,6 +530,18 @@ static SILValue getSub(SILLocation Loc, SILValue Val, unsigned SubVal, | |
| return B.createTupleExtract(Loc, AI, 0); | ||
| } | ||
|
|
||
| static SILValue getAdd(SILLocation Loc, SILValue Val, unsigned AddVal, | ||
| SILBuilder &B) { | ||
| SmallVector<SILValue, 4> Args(1, Val); | ||
| Args.push_back(B.createIntegerLiteral(Loc, Val->getType(), AddVal)); | ||
| Args.push_back(B.createIntegerLiteral( | ||
| Loc, SILType::getBuiltinIntegerType(1, B.getASTContext()), -1)); | ||
|
|
||
| auto *AI = B.createBuiltinBinaryFunctionWithOverflow( | ||
| Loc, "sadd_with_overflow", Args); | ||
| return B.createTupleExtract(Loc, AI, 0); | ||
| } | ||
|
|
||
| /// A canonical induction variable incremented by one from Start to End-1. | ||
| struct InductionInfo { | ||
| SILArgument *HeaderVal; | ||
|
|
@@ -552,12 +564,12 @@ struct InductionInfo { | |
|
|
||
| SILInstruction *getInstruction() { return Inc; } | ||
|
|
||
| SILValue getFirstValue(SILLocation &Loc, SILBuilder &B, unsigned AddVal) { | ||
| return AddVal != 0 ? getAdd(Loc, Start, AddVal, B) : Start; | ||
| } | ||
|
|
||
| SILValue getLastValue(SILLocation &Loc, SILBuilder &B, unsigned SubVal) { | ||
| return SubVal != 0 ? getSub(Loc, End, SubVal, B) : End; | ||
| } | ||
|
|
||
| /// If necessary insert an overflow for this induction variable. | ||
|
|
@@ -718,28 +730,62 @@ static bool isGuaranteedToBeExecuted(DominanceInfo *DT, SILBasicBlock *Block, | |
| /// induction variable. | ||
| class AccessFunction { | ||
| InductionInfo *Ind; | ||
| bool preIncrement; | ||
|
|
||
| AccessFunction(InductionInfo *I, bool isPreIncrement = false) | ||
| : Ind(I), preIncrement(isPreIncrement) {} | ||
|
|
||
| public: | ||
|
|
||
| operator bool() { return Ind != nullptr; } | ||
|
|
||
| static AccessFunction getLinearFunction(SILValue Idx, | ||
| InductionAnalysis &IndVars) { | ||
| // Match the actual induction variable buried in the integer struct. | ||
| // bb(%ivar) | ||
| // %2 = struct $Int(%ivar : $Builtin.Word) | ||
| // = apply %check_bounds(%array, %2) : | ||
| // or | ||
| // bb(%ivar1) | ||
| // %ivar2 = builtin "sadd_with_overflow_Int64"(%ivar1,...) | ||
| // %t = tuple_extract %ivar2 | ||
| // %s = struct $Int(%t : $Builtin.Word) | ||
| // = apply %check_bounds(%array, %s) : | ||
|
|
||
| bool preIncrement = false; | ||
|
|
||
| auto ArrayIndexStruct = dyn_cast<StructInst>(Idx); | ||
| if (!ArrayIndexStruct) | ||
| return nullptr; | ||
|
|
||
| auto AsArg = | ||
| dyn_cast<SILArgument>(ArrayIndexStruct->getElements()[0]); | ||
|
|
||
| if (!AsArg) { | ||
| auto *TupleExtract = | ||
| dyn_cast<TupleExtractInst>(ArrayIndexStruct->getElements()[0]); | ||
|
|
||
| if (!TupleExtract) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| auto *Builtin = dyn_cast<BuiltinInst>(TupleExtract->getOperand()); | ||
| if (!Builtin || Builtin->getBuiltinKind() != BuiltinValueKind::SAddOver) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| // We don't check if the second argument to the builtin is loop invariant | ||
| // here, because only induction variables with a +1 incremenent are | ||
| // considered for bounds check optimization. | ||
| AsArg = dyn_cast<SILArgument>(Builtin->getArguments()[0]); | ||
|
There was a problem hiding this comment. Don't you need to check if the second argument is a loop-invariant constant? There was a problem hiding this comment. I thought it was okay because There was a problem hiding this comment. I'll write some tests for this case. There was a problem hiding this comment. I see, the Alternatively, you can replace that argument check with a constant-check of the second argument (as I suggested). Then the optimization could also handle loops like There was a problem hiding this comment. @eeckstein I'll add the increment by other constants in a different PR. There are other unhandled patterns as well. Eg: Local array patterns |
||
| if (!AsArg) { | ||
| return nullptr; | ||
| } | ||
| preIncrement = true; | ||
| } | ||
|
|
||
| if (auto *Ind = IndVars[AsArg]) | ||
| return AccessFunction(Ind, preIncrement); | ||
|
|
||
| return nullptr; | ||
| } | ||
|
|
@@ -759,7 +805,7 @@ class AccessFunction { | |
| SILBuilderWithScope Builder(Preheader->getTerminator(), AI); | ||
|
|
||
| // Get the first induction value. | ||
| auto FirstVal = Ind->getFirstValue(Loc, Builder, preIncrement ? 1 : 0); | ||
| // Clone the struct for the start index. | ||
| auto Start = cast<SingleValueInstruction>(CheckToHoist.getIndex()) | ||
| ->clone(Preheader->getTerminator()); | ||
|
|
@@ -771,7 +817,7 @@ class AccessFunction { | |
| NewCheck->setOperand(1, Start); | ||
|
|
||
| // Get the last induction value. | ||
| auto LastVal = Ind->getLastValue(Loc, Builder, preIncrement ? 0 : 1); | ||
| // Clone the struct for the end index. | ||
| auto End = cast<SingleValueInstruction>(CheckToHoist.getIndex()) | ||
| ->clone(Preheader->getTerminator()); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't clear to me whether the meaning of
getLinearFunctionchanges depending on whether the bounds check is on the preincremented or postincremented induction variable. I think that needs to be specified. I suggest testing the boundary conditions to be sure there is no off-by-one bug.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is a off-by-one bug. So far, the AccessFunction was the identity function. But with your change it can add the offset of the increment. You need to add that info to AccessFunction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a check in your tests for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eeckstein I added check lines with the bounds check now