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

[X86] Infinite loop in DAG combiner for targets with the fast-lzcnt feature #54694

Closed
goussepi opened this issue Apr 1, 2022 · 13 comments
Closed

[X86] Infinite loop in DAG combiner for targets with the fast-lzcnt feature #54694

goussepi opened this issue Apr 1, 2022 · 13 comments
Labels
backend:X86 bug Indicates an unexpected problem or unintended behavior release:backport release:merged release:reviewed
Milestone
LLVM 14.0.2 Release

Comments

@goussepi
Copy link
Collaborator

goussepi commented Apr 1, 2022 

define i32 @testmasks(i32 %0, i32 %1) {
  %3 = and i32 %0, 131072
  %4 = icmp eq i32 %3, 0
  %5 = and i32 %1, 131072
  %6 = icmp eq i32 %5, 0
  %7 = select i1 %4, i1 true, i1 %6
  %8 = select i1 %7, i32 1, i32 2
  ret i32 %8
}

llc -mtriple=x86_64-pc-linux -mcpu=btver2 mask.ll -O3 -o -
.text
.file "lzcnt2.ll"
@goussepi goussepi added bug Indicates an unexpected problem or unintended behavior backend:X86 tools:llc labels Apr 1, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 1, 2022

@llvm/issue-subscribers-bug

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 1, 2022

@llvm/issue-subscribers-backend-x86

@RKSimon RKSimon removed the tools:llc label Apr 1, 2022
@goussepi goussepi changed the title [X86] Infinite loop in DAB combiner for targets with the fast-lzcnt feature [X86] Infinite loop in DAG combiner for targets with the fast-lzcnt feature Apr 1, 2022
goussepi added a commit that referenced this issue Apr 5, 2022
@goussepi
[x86] Fix infinite loop inside DAG combiner with lzcnt feature.
a3d5f1c 
The issue affects targets supporting fast-lzcnt such as btver2.
This removes extraneous zext/trunc node insertions to fix the infinite
loop.
This fixes Issue #54694

Differential Revision: https://reviews.llvm.org/D122900

Reviewed By: RKSimon, spatel, lebedev.ri
@RKSimon
Copy link
Collaborator

RKSimon commented Apr 8, 2022

Fixed by a3d5f1c - @goussepi should we cherry pick this for 14.x ?

@RKSimon RKSimon added this to the LLVM 14.0.1 Release milestone Apr 8, 2022
@RKSimon
Copy link
Collaborator

RKSimon commented Apr 8, 2022

/cherry-pick a3d5f1c

@RKSimon
Copy link
Collaborator

RKSimon commented Apr 8, 2022

(discussed this with @goussepi offline)

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 8, 2022

/branch llvmbot/llvm-project/issue54694

llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Apr 8, 2022
@goussepi @llvmbot
[x86] Fix infinite loop inside DAG combiner with lzcnt feature.
62e2257 
The issue affects targets supporting fast-lzcnt such as btver2.
This removes extraneous zext/trunc node insertions to fix the infinite
loop.
This fixes Issue llvm#54694

Differential Revision: https://reviews.llvm.org/D122900

Reviewed By: RKSimon, spatel, lebedev.ri

(cherry picked from commit a3d5f1c)
@llvmbot llvmbot mentioned this issue Apr 8, 2022
Open
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 8, 2022

/pull-request llvmbot#159

@tstellar
Copy link
Collaborator

@RKSimon @goussepi There is a test failure with this patch in the release/14.x: llvmbot#159

Can someone update the patch and submit a branch.

@goussepi
Copy link
Collaborator Author

Thank you @tstellar for reporting, I am having a look at it.

wjristow pushed a commit to wjristow/llvm-project that referenced this issue Apr 20, 2022
@goussepi @wjristow
[x86] Fix infinite loop inside DAG combiner with lzcnt feature.
d735a73 
The issue affects targets supporting fast-lzcnt such as btver2.
This removes extraneous zext/trunc node insertions to fix the infinite
loop.
This fixes Issue llvm#54694

Differential Revision: https://reviews.llvm.org/D122900

Reviewed By: RKSimon, spatel, lebedev.ri

(cherry picked from commit a3d5f1c)
Signed-off-by: Warren Ristow <warren.ristow@sony.com>

In https://reviews.llvm.org/D122900 a new function (to exercise the
infinite-loop bug) was added to llvm/test/CodeGen/X86/lzcnt-zext-cmp.ll.
In applying the fix in the main branch, two previously existing
functions in that test also changed behavior slightly, and in the review
it was noted:
    The instructions generated end up being reordered in some cases
    but I think it is equivalent.
That reordering did not happen in those pre-existing functions when
applying the fix to the slightly older code-base of the llvm14 branch,
and so they are suppressed here.  So the updated version of the test in
this commit has the additional function added to it, but it is otherwise
identical to the previous llvm14 version of the test.
@wjristow
Copy link
Collaborator

/branch wjristow/llvm-project/pr54694

@llvmbot llvmbot mentioned this issue Apr 20, 2022
Merged
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 20, 2022

/pull-request llvmbot#177

@wjristow
Copy link
Collaborator

Discussed this with @goussepi and he asked me to take care of the branch

tstellar pushed a commit to llvmbot/llvm-project that referenced this issue Apr 21, 2022
@goussepi @tstellar
[x86] Fix infinite loop inside DAG combiner with lzcnt feature.
b83c4a2 
The issue affects targets supporting fast-lzcnt such as btver2.
This removes extraneous zext/trunc node insertions to fix the infinite
loop.
This fixes Issue llvm#54694

Differential Revision: https://reviews.llvm.org/D122900

Reviewed By: RKSimon, spatel, lebedev.ri

(cherry picked from commit a3d5f1c)
Signed-off-by: Warren Ristow <warren.ristow@sony.com>

In https://reviews.llvm.org/D122900 a new function (to exercise the
infinite-loop bug) was added to llvm/test/CodeGen/X86/lzcnt-zext-cmp.ll.
In applying the fix in the main branch, two previously existing
functions in that test also changed behavior slightly, and in the review
it was noted:
    The instructions generated end up being reordered in some cases
    but I think it is equivalent.
That reordering did not happen in those pre-existing functions when
applying the fix to the slightly older code-base of the llvm14 branch,
and so they are suppressed here.  So the updated version of the test in
this commit has the additional function added to it, but it is otherwise
identical to the previous llvm14 version of the test.
@tstellar
Copy link
Collaborator

tstellar commented Apr 25, 2022 •
edited
Loading

Merged: b83c4a2

mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
@goussepi
[x86] Fix infinite loop inside DAG combiner with lzcnt feature.
14da4ac 
The issue affects targets supporting fast-lzcnt such as btver2.
This removes extraneous zext/trunc node insertions to fix the infinite
loop.
This fixes Issue llvm/llvm-project#54694

Differential Revision: https://reviews.llvm.org/D122900

Reviewed By: RKSimon, spatel, lebedev.ri
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backend:X86 bug Indicates an unexpected problem or unintended behavior release:backport release:merged release:reviewed
Projects
LLVM Release Status
Archived in project
Milestone
LLVM 14.0.2 Release
Development

No branches or pull requests
5 participants
@tstellar @goussepi @RKSimon @wjristow @llvmbot