Skip to content

Commit

Permalink
[SLP][NFC]Walk over entries, not single values.
Browse files Browse the repository at this point in the history 
Better to walk over SLP nodes rather than single values. Matching
a value to a node is not a 1-to-1 relation, one value may be part of
several nodes and compiler may get wrong node, when trying to map it.
Currently there are no such issues detected, but they may appear in
future.
  • Loading branch information
@alexey-bataev
alexey-bataev committed Apr 10, 2024
1 parent 54a9f00 commit 938a734
Showing 1 changed file with 167 additions and 172 deletions.

3 comments on commit 938a734

@mikaelholmen
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexey-bataev

This patch doesn't seem to be NFC, in fact it causes a miscompile.
Reproduce with

opt -mtriple=aarch64 -passes=slp-vectorizer bbi-95270.ll -S -o - -slp-threshold=-100

Before this patch we get

define i16 @foo() {
entry:
  %lnot = xor i1 true, true
  %lnot.ext = zext i1 %lnot to i16
  %add = add nsw i16 0, %lnot.ext
  %lnot5 = xor i1 true, true
  %lnot.ext6 = zext i1 %lnot5 to i16
  %add7 = add nsw i16 %add, %lnot.ext6
  ret i16 %add7
}

and after

define i16 @foo() {
entry:
  %lnot = xor i1 false, true
  %lnot.ext = zext i1 %lnot to i16
  %add = add nsw i16 0, %lnot.ext
  %lnot5 = xor i1 false, true
  %lnot.ext6 = zext i1 %lnot5 to i16
  %add7 = add nsw i16 %add, %lnot.ext6
  ret i16 %add7
}

Note the difference in the "xor" instructions.
bbi-95270.ll.gz

@mikaelholmen
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote an issue about the miscompile #91309

@alexey-bataev
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexey-bataev

This patch doesn't seem to be NFC, in fact it causes a miscompile. Reproduce with

opt -mtriple=aarch64 -passes=slp-vectorizer bbi-95270.ll -S -o - -slp-threshold=-100

Before this patch we get

define i16 @foo() {
entry:
  %lnot = xor i1 true, true
  %lnot.ext = zext i1 %lnot to i16
  %add = add nsw i16 0, %lnot.ext
  %lnot5 = xor i1 true, true
  %lnot.ext6 = zext i1 %lnot5 to i16
  %add7 = add nsw i16 %add, %lnot.ext6
  ret i16 %add7
}

and after

define i16 @foo() {
entry:
  %lnot = xor i1 false, true
  %lnot.ext = zext i1 %lnot to i16
  %add = add nsw i16 0, %lnot.ext
  %lnot5 = xor i1 false, true
  %lnot.ext6 = zext i1 %lnot5 to i16
  %add7 = add nsw i16 %add, %lnot.ext6
  ret i16 %add7
}

Note the difference in the "xor" instructions. bbi-95270.ll.gz

The patch itself is definitely NFC. If there is a bug after it, it means the bug existed before and the patch just revealed it. I'll fix it ASAP

Please sign in to comment.