Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

<numeric>: transform_inclusive_scan passes output values to the binary (reduce) operation #3783

Closed
akrivx opened this issue Jun 17, 2023 · 2 comments · Fixed by #4701
Closed

<numeric>: transform_inclusive_scan passes output values to the binary (reduce) operation #3783

akrivx opened this issue Jun 17, 2023 · 2 comments · Fixed by #4701
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@akrivx
Copy link

akrivx commented Jun 17, 2023 •
edited by CaseyCarter
Loading

Describe the bug

The overload of transform_inclusive_scan that takes execution policy and initial value arguments passes the result of dereferencing the output iterator to the binary reduce operation.

Doing so requires that the output value type can be converted to the types expected by the binary operation.

Command-line test case

See this example here: https://godbolt.org/z/4fscYPcTW
It's working fine with the latest gcc.

Expected behavior

transform_inclusive_scan does not pass the result of dereferencing the output iterator to the binary operation.

STL version

Microsoft Visual Studio Community 2022 (64-bit)
Version 17.6.2
@StephanTLavavej StephanTLavavej changed the title numeric: transform_inclusive_scan passes output values to the binary (reduce) operation <numeric>: transform_inclusive_scan passes output values to the binary (reduce) operation Jun 17, 2023
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Jun 17, 2023
@achabense
Copy link
Contributor

achabense commented Jun 18, 2023 •
edited
Loading

&& transform_exclusive_scan(execution::par,...) has the same problem.

It seems the usage of _Apply_inclusive_predecessor is wrong here.

STL/stl/inc/execution

Lines 4841 to 4847 in 15aea98

// Apply the predecessor overall sum to current overall sum and elements
if (_Prev_chunk->_Get_available_state() & _Sum_available) { // predecessor overall sum done, use directly
_Chunk->_Apply_inclusive_predecessor(_Prev_chunk->_Sum._Ref(), _Dest, _Last, _Reduce_op);
} else {
auto _Tmp = _Get_lookback_sum(_Prev_chunk, _Reduce_op);
_Chunk->_Apply_inclusive_predecessor(_Tmp, _Dest, _Last, _Reduce_op);
}

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Oct 25, 2023 •
edited
Loading

It seems that we should pass _Transform_op to _Apply_inclusive_predecessor (with its signature changed) and use _Transform_op in it.

We should also pass the beginning "input" forward iterator. Perhaps we should avoid moving.

Edit: The standard doesn't seem to require parts of output range (whose element type is the value type of ForwardIterator2) to be usable as mediate results (whose element type should be T). Perhaps we need to create a new T sequence when it's impossible to reuse the output range.

@frederick-vs-ja frederick-vs-ja mentioned this issue Feb 23, 2024
Merged
Andor233 added a commit to Andor233/STL that referenced this issue May 30, 2024
@Andor233
Fix microsoft#3783
d9f860f 
<execution>:
transform_inclusive_scan
transform_exclusive_scan
exclusive_scan
inclusive_scan
passes output values to the binary (reduce) operation with par policy
@Andor233 Andor233 mentioned this issue May 31, 2024
Merged
@StephanTLavavej StephanTLavavej added the fixed Something works now, yay! label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Milestone
No milestone
4 participants
@StephanTLavavej @frederick-vs-ja @achabense @akrivx