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

[LoopVectorize] Failure to compare multiple inputs as the same (splatted) value #62565

Closed
RKSimon opened this issue May 5, 2023 · 9 comments · Fixed by #78304
Closed

[LoopVectorize] Failure to compare multiple inputs as the same (splatted) value #62565

RKSimon opened this issue May 5, 2023 · 9 comments · Fixed by #78304
Labels
bug Indicates an unexpected problem or unintended behavior loopoptim miscompilation

Comments

@RKSimon
Copy link
Collaborator

RKSimon commented May 5, 2023 •
edited

Pulled out of a regression report on https://reviews.llvm.org/rGfed28ada47f5 / fed28ad

I haven't done a great job with reducing this - but this shows the effect - https://gcc.godbolt.org/z/5q36ExYqa - loop-vectorize creates the icmp ne <2 x i64> %24, undef and then instcombine simplifies it all - but I'm still not certain that LV is at fault or not @fhahn do you have any suggestions?

That's a very interesting case! It looks like this is indeed a LV miscompile. The issue is that we introduce multiple new uses of undef. The issue is that each use of undef could take different values, whereas the code we generate relies that we compare against the same value. See https://alive2.llvm.org/ce/z/osWdyX for Alive2 showing the issue (the input is a further reduced version of the test case)

I think we need to insert a freeze, then broadcast it to a vector and consistently use that in the generated code instead of undef: https://alive2.llvm.org/ce/z/HT7J6u. I can look into a patch for this

FTR this also is an issue for arbitrary inputs, not just poison/undef constants: https://alive2.llvm.org/ce/z/5BsKqY

CC @fhahn @alexfh
@RKSimon RKSimon added bug Indicates an unexpected problem or unintended behavior llvm:codegen loopoptim labels May 5, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented May 5, 2023

@llvm/issue-subscribers-bug

@alexfh
Copy link
Contributor

alexfh commented May 5, 2023

The link to the discussion is broken for some reason. Trying again: https://reviews.llvm.org/rGfed28ada47f56ef775ab966e69a8842dcd315fad

fhahn added a commit that referenced this issue Jun 22, 2023
@fhahn
[LV] Add test with reduction start values that are/may be poison/undef.
dc9f69e 
Test cases for #62565.
@fhahn
Copy link
Contributor

fhahn commented Jun 24, 2023

Proposed a fix: https://reviews.llvm.org/D153697

@alinas
Copy link
Contributor

alinas commented Oct 17, 2023

Pinging this to (hopefully) get the proposed patch fixes back up to top of inbox for review.

@rnk
Copy link
Collaborator

rnk commented Jan 12, 2024

It looks like this is still open. @fhahn , mind making a pull request for this now that Phab is readonly?

@fhahn
Copy link
Contributor

fhahn commented Jan 14, 2024

Will do, with the suggested changes from the original review

fhahn added a commit to fhahn/llvm-project that referenced this issue Jan 16, 2024
@fhahn
[LV] Improve AnyOf reduction codegen.
9846f97 
Update AnyOf reduction code generation to only keep track of the AnyOf
property in a boolean vector in the loop, only selecting either the new
or start value in the middle block.

This fixes the llvm#62565, as now there aren't multiple uses of the
start/new values.

Fixes llvm#62565
@fhahn fhahn mentioned this issue Jan 16, 2024
Merged
@fhahn
Copy link
Contributor

fhahn commented Jan 16, 2024

PR is ready now: #78304

@rnk
Copy link
Collaborator

rnk commented Mar 15, 2024

Reverted in 589c7ab

@rnk rnk reopened this Mar 15, 2024
@fhahn
Copy link
Contributor

fhahn commented Mar 18, 2024

Looks like this was closed again by a commit in a for of llvm-project, which is a bit surprising. Re-opening again

@fhahn fhahn reopened this Mar 18, 2024
@fhahn fhahn closed this as completed in c6e38b9 Apr 5, 2024
fhahn added a commit that referenced this issue May 3, 2024
@fhahn
Reapply "[LV] Improve AnyOf reduction codegen. (#78304)"
bccb7ed 
This reverts the revert commit c6e0162.

This patch includes a fix for any-of reductions and epilogue
vectorization. Extra test coverage for the issue that caused the revert
has been added in bce3bfc and an assertion has been added in
c7209cb.

--------------------------------
Original commit message:

Update AnyOf reduction code generation to only keep track of the AnyOf
property in a boolean vector in the loop, only selecting either the new
or start value in the middle block.

The patch incorporates feedback from https://reviews.llvm.org/D153697.

This fixes the #62565, as now there aren't multiple uses of the
start/new values.

Fixes #62565

PR: #78304
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Indicates an unexpected problem or unintended behavior loopoptim miscompilation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[LV] Improve AnyOf reduction codegen. fhahn/llvm-project
7 participants
@rnk @fhahn @alinas @RKSimon @alexfh @EugeneZelenko @llvmbot