Call visitor only in one place in bodies of folds for RangeInclusive

[AngelicosPhosphoros]

View all comments

Prior attempt: eb5b096

I rewrote the loops in spec_try_fold and spec_try_rfold to call argument function only inside a loop body instead of having extra single call after the loop.

I expect this change to make the loop body a single callsite for lambdas passed into closed range iterators (x..=y). In such cases, it would encourage LLVM to inline the lambda and remove the instantiation of its function. Also, it would reduce code of try_fold calls where LLVM previously inlined the lambda body twice.

I handled the danger of overflow or underflow by switching from while loops to loops with postcondition.

I also checked manually that the LLVM do not fail to optimize loops.

Example test (compiled using rustc +stage1 .\sum.rs --crate-type=dylib -O --emit=llvm-ir):

pub fn mysum(a:u32, b: u32)->u32{
    (a..=b).sum()
}

Output was successfully folded into arithmetic progression formula without loops:

define noundef i32 @mysum(i32 noundef %a, i32 noundef %b) {
bb28.i.i:
  %_0.i.not.i.i = icmp ugt i32 %a, %b
  br i1 %_0.i.not.i.i, label %_<fun_exit>.exit, label %bb4.preheader.i.i

bb4.preheader.i.i:                                ; preds = %bb28.i.i
  %_0.i510.i.i = icmp eq i32 %a, %b
  br i1 %_0.i510.i.i, label %bb16.i.i, label %bb9.preheader.i.i

bb9.preheader.i.i:                                ; preds = %bb4.preheader.i.i
  %0 = xor i32 %a, -1
  %1 = add i32 %b, %0
  %2 = add i32 %a, 1
  %3 = mul i32 %1, %2
  %4 = add i32 %3, %a
  %5 = zext i32 %1 to i33
  %reass.sub = sub i32 %b, %a
  %6 = add i32 %reass.sub, -2
  %7 = zext i32 %6 to i33
  %8 = mul i33 %5, %7
  %9 = lshr i33 %8, 1
  %10 = trunc nuw i33 %9 to i32
  %11 = add i32 %4, %10
  br label %bb16.i.i

bb16.i.i:                                         ; preds = %bb9.preheader.i.i, %bb4.preheader.i.i
  %i.sroa.0.0.lcssa.i.i = phi i32 [ %b, %bb9.preheader.i.i ], [ %a, %bb4.preheader.i.i ]
  %accum.sroa.0.0.lcssa.i.i = phi i32 [ %11, %bb9.preheader.i.i ], [ 0, %bb4.preheader.i.i ]
  %_4.0.i.i8.i.i = add i32 %accum.sroa.0.0.lcssa.i.i, %i.sroa.0.0.lcssa.i.i
  br label %_<fun_exit>.exit

_<fun_exit>.exit: ; preds = %bb28.i.i, %bb16.i.i
  %_0.sroa.0.0.i.i = phi i32 [ %_4.0.i.i8.i.i, %bb16.i.i ], [ 0, %bb28.i.i ]
  ret i32 %_0.sroa.0.0.i.i
}

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 7 candidates

[AngelicosPhosphoros]

Is there a way to add codegen test that checks that output has no loops?

[Kobzol]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 964a43b (964a43b3b2b4ee75682f09de2b9be2af0f815b73, parent: ba0949ab745985a442e274ba52e8fb348cb0c662)

[rust-timer]

Finished benchmarking commit (964a43b): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -1.7%, secondary -2.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.4%	[3.4%, 3.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.4%	[-3.9%, -2.7%]	3
Improvements ✅ (secondary)	-2.7%	[-2.7%, -2.7%]	1
All ❌✅ (primary)	-1.7%	[-3.9%, 3.4%]	4

Cycles

Results (primary 2.6%, secondary 6.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	6.5%	[2.5%, 10.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

Binary size

Results (primary -0.1%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	1

Bootstrap: 511.658s -> 512.106s (0.09%)
Artifact size: 398.51 MiB -> 400.57 MiB (0.52%)

[ZuseZ4]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: c41e97d (c41e97d292028c434505069ffaadb398208a84a2, parent: e96c36b6f76833388c519561d145492d2c08db4e)

[rust-timer]

Finished benchmarking commit (c41e97d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.1%, secondary 0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	4.9%	[4.9%, 4.9%]	1
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	1
All ❌✅ (primary)	1.1%	[-2.7%, 4.9%]	2

Cycles

Results (secondary -6.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.2%	[-6.2%, -6.2%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.1%, 0.0%]	5

Bootstrap: 530.554s -> 514.219s (-3.08%)
Artifact size: 400.60 MiB -> 400.51 MiB (-0.02%)

[AngelicosPhosphoros]

r? libs

[rustbot]

The review request libs corresponds to 7 reviewer(s).
However, none of them are available for a review at this time.
Use r? to specify someone else to assign.

[AngelicosPhosphoros]

r? libs

[rustbot]

The review request libs corresponds to 7 reviewer(s).
However, none of them are available for a review at this time.
Use r? to specify someone else to assign.

[Mark-Simulacrum]

Is there a way to add codegen test that checks that output has no loops?

Yes, you should be able to add a test somewhere under lib-optimizations in codegen-llvm I think. See e.g. https://github.com/rust-lang/rust/blob/main/tests/codegen-llvm/lib-optimizations/iter-sum.rs

I'm not sure how easy it'll be to guarantee no loops, but you may be able to come up with some form of CHECK-NOT for br perhaps? I suspect any test will be somewhat imprecise but maybe better than nothing if we can get it to be non-flaky.

[Mark-Simulacrum]

Seems like there's no meaningful perf-measured improvement from this. Do you think that the extra implementation complexity is still justified? I think I could go either way.

Implementation wise this looks fine to me, thanks for adding some more test coverage!