Handle a gap while matching SIL patterns for hoisting array bound checks #65573
Conversation
|
@swift-ci test |
| return nullptr; | ||
| } | ||
|
|
||
| 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?
And handle that in isZeroCount and hoistCheckToPreheader?
There was a problem hiding this comment.
I thought it was okay because InductionAnalysis bails out for values that are not integer constant 1 ? https://github.com/apple/swift/blob/dd813af0a7670f420d30aa95326863fbb772463a/lib/SILOptimizer/LoopTransforms/ArrayBoundsCheckOpts.cpp#L650
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 dyn_cast<SILArgument> makes sure that the add-builtin is matching the increment of the induction variable. Can you add a comment which makes that clear?
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
for i in 0..<10 {
a[i + c] // c is a constant
}
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
|
@swift-ci test |
|
@swift-ci Please Apple Silicon benchmark |
| return %t : $() | ||
| } | ||
|
|
||
| // RANGECHECK-LABEL: sil @hoist_new_ind_pattern2 : |
There was a problem hiding this comment.
Could you add the pseudo-code for hoist_new_ind_pattern2 so we can see what's being tested and add comments on why incrementing by two prevents bounds check elimination?
| // { | ||
| // index = index + index | ||
| // ...a[index]... | ||
| // } |
There was a problem hiding this comment.
Could you add a comment on why bounds check hoisting fails for hoist_new_ind_pattern3?
| } | ||
|
|
||
| auto *Builtin = dyn_cast<BuiltinInst>(TupleExtract->getOperand()); | ||
| if (!Builtin || Builtin->getBuiltinKind() != BuiltinValueKind::SAddOver) { |
There was a problem hiding this comment.
It isn't clear to me whether the meaning of getLinearFunction changes 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.
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.
Thanks! I'll fix this
There was a problem hiding this comment.
Can you also add a check in your tests for this?
There was a problem hiding this comment.
@eeckstein I added check lines with the bounds check now
| } | ||
|
|
||
| auto *Builtin = dyn_cast<BuiltinInst>(TupleExtract->getOperand()); | ||
| if (!Builtin || Builtin->getBuiltinKind() != BuiltinValueKind::SAddOver) { |
There was a problem hiding this comment.
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.
| return nullptr; | ||
| } | ||
|
|
||
| AsArg = dyn_cast<SILArgument>(Builtin->getArguments()[0]); |
There was a problem hiding this comment.
I see, the dyn_cast<SILArgument> makes sure that the add-builtin is matching the increment of the induction variable. Can you add a comment which makes that clear?
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
for i in 0..<10 {
a[i + c] // c is a constant
}
|
@swift-ci test |
test/SILOptimizer/abcopts.sil
Outdated
| // RANGECHECK: [[ZERO:%.*]] = integer_literal $Builtin.Int32, 0 | ||
| // RANGECHECK: [[INTZERO:%.*]] = struct $Int32 ([[ZERO]] : $Builtin.Int32) | ||
| // RANGECHECK: [[FOUR:%.*]] = integer_literal $Builtin.Int32, 4 | ||
| // RANGECHECK: [[A1:%.*]] = apply [[CB]]([[INTZERO]], [[TRUE]], %0) : $@convention(method) (Int32, Bool, @owned ArrayInt) -> _DependenceToken |
There was a problem hiding this comment.
Wait, shouldn't this be 1? The constant increment must also be added to the lower-bound range check value.
There was a problem hiding this comment.
Yes, you are right. I added the fix. Thank you
|
@swift-ci test |
Enables hoisting bounds checks from simple loops like :
for i in 0...4 { ...a[i]... }