[DAG] SelectionDAG.computeKnownBits - add NSW/NUW flags support to ISD::SHL handling #89877

zxc12523 · 2024-04-24T06:15:17Z

github-actions · 2024-04-24T06:15:38Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-04-24T06:16:13Z

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-selectiondag

Author: None (zxc12523)

Changes

fix #89414

Full diff: https://github.com/llvm/llvm-project/pull/89877.diff

1 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+9-2)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 5de5587b38fa1c..bde6d654fee347 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -3492,16 +3492,23 @@ KnownBits SelectionDAG::computeKnownBits(SDValue Op, const APInt &DemandedElts,
       Known.Zero.setBitsFrom(1);
     break;
   }
-  case ISD::SHL:
+  case ISD::SHL: {
     Known = computeKnownBits(Op.getOperand(0), DemandedElts, Depth + 1);
     Known2 = computeKnownBits(Op.getOperand(1), DemandedElts, Depth + 1);
-    Known = KnownBits::shl(Known, Known2);
+
+    bool NUW = Op.getNode()->Flags.hasNoUnsignedWrap();
+    bool NSW = Op.getNode()->Flags.hasNoSignedWrap();
+    
+    bool ShAmtNonZero = Known2.isNonZero();
+
+    Known = KnownBits::shl(Known, Known2, NUW, NSW, ShAmtNonZero);
 
     // Minimum shift low bits are known zero.
     if (const APInt *ShMinAmt =
             getValidMinimumShiftAmountConstant(Op, DemandedElts))
       Known.Zero.setLowBits(ShMinAmt->getZExtValue());
     break;
+  }
   case ISD::SRL:
     Known = computeKnownBits(Op.getOperand(0), DemandedElts, Depth + 1);
     Known2 = computeKnownBits(Op.getOperand(1), DemandedElts, Depth + 1);

arsenm

Needs tests

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

github-actions · 2024-04-26T10:40:58Z

✅ With the latest revision this PR passed the C/C++ code formatter.

arsenm · 2024-04-26T12:56:40Z

llvm/test/CodeGen/X86/fold-int-pow2-with-fmul-or-fdiv.ll

@@ -1668,4 +1668,4 @@ define float @fdiv_pow_shl_cnt32_okay(i32 %cnt) nounwind {
  %conv = uitofp i32 %shl to float
  %mul = fdiv float 0x3a20000000000000, %conv
  ret float %mul
-}
+}


end of file newline error

Still needs an extra empty line at the EOF

arsenm · 2024-04-26T12:57:28Z

llvm/test/CodeGen/X86/pr89877.ll

+; X64-NEXT:    movzwl	%ax, %eax
+; X64-NEXT:    rep bsfl %eax, %eax
+; X64-NEXT:    retq
+  %x = shl nuw nsw i16 256, %xx


Tests should stress the individual nuw/nsw flags. Also should have some negative tests where the required flag is missing

Can u give me an example of negative test?

The same test, just with flags removed that shows your patch made no change

Will I also need to add test case for ShAmtNonZero flag?

arsenm · 2024-04-26T12:58:12Z

llvm/test/CodeGen/X86/pr89877.ll

+; RUN: llc < %s -mtriple=x86_64-unknown-unknown | FileCheck %s --check-prefixes=CHECK-SSE
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx2 | FileCheck %s --check-prefixes=CHECK-AVX2
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx512f | FileCheck %s --check-prefixes=CHECK-NO-FASTFMA
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mcpu=skx -fp-contract=fast | FileCheck %s --check-prefixes=CHECK-FMA


-fp-contract flag shouldn't be necessary for the purposes of the test (if it were, you would be better of applying the contract flag to the individual instructions)

When I run "ninja check-llvm-codegen-x86", I found some tasks fail due to the modification. One of those is fold-int-pow2-with-fmul-or-fdiv.ll, which contain "-fp-contract=fast" in the testing command. So, I guess it is not related to this.

RKSimon · 2024-04-29T09:58:55Z

llvm/test/CodeGen/X86/fold-int-pow2-with-fmul-or-fdiv.ll

@@ -3,6 +3,7 @@
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx2 | FileCheck %s --check-prefixes=CHECK-AVX,CHECK-AVX2
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx512f | FileCheck %s --check-prefixes=CHECK-AVX,CHECK-AVX512F,CHECK-NO-FASTFMA
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mcpu=skx -fp-contract=fast | FileCheck %s --check-prefixes=CHECK-AVX,CHECK-AVX512F,CHECK-FMA
+; XFAIL: *


remove this

RKSimon · 2024-04-29T09:58:59Z

llvm/test/CodeGen/X86/known-never-zero.ll

@@ -1,6 +1,7 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=i686-unknown-unknown -mattr=+sse2 | FileCheck %s --check-prefixes=X86
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx | FileCheck %s --check-prefixes=X64
+; XFAIL: *


remove this

RKSimon · 2024-04-29T09:59:39Z

llvm/test/CodeGen/X86/pr89877.ll

+; X86-NEXT:	   retl
+; X86-NEXT:	 .LBB0_1:
+; X86-NEXT:	   movl	$32, %eax
+; X86-NEXT:	   retl


this indentation looks off - was this generated by the update_llc_test_checks script?

RKSimon

According to the CI you need to regenerate fold-int-pow2-with-fmul-or-fdiv.ll and known-never-zero.ll

RKSimon

LGTM

github-actions · 2024-05-02T09:32:16Z

@zxc12523 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

zxc12523 and others added 2 commits April 24, 2024 13:57

feat: add NSW & NUW & ShAmtNonZero

ea74bf2

Merge branch 'llvm:main' into 89414

c22ef9f

llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Apr 24, 2024

arsenm reviewed Apr 24, 2024

View reviewed changes

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Outdated Show resolved Hide resolved

arsenm requested a review from RKSimon April 24, 2024 07:33

add new codegen

1b5d177

llvmbot added the backend:X86 label Apr 26, 2024

create codegen test: pr89877.ll

8578c44

arsenm reviewed Apr 26, 2024

View reviewed changes

Test individual nuw/nsu flags & Add expect fail to other .ll

addd9c0

RKSimon reviewed Apr 29, 2024

View reviewed changes

zxc12523 added 2 commits April 30, 2024 14:29

Change Op.getNode()->Flags to Op->getFlags() & Remove XFail

ba07a98

Fix indentation in pr89877.ll

2bd187b

RKSimon requested changes Apr 30, 2024

View reviewed changes

Update fold-int-pow2-with-fmul-or-fdiv.ll & known-never-zero.ll

cb72268

zxc12523 requested a review from RKSimon May 1, 2024 07:29

fix newline in fold-int-pow2-with-fmul-or-fdiv.ll

ae73713

RKSimon approved these changes May 1, 2024

View reviewed changes

RKSimon merged commit 171aeb2 into llvm:main May 2, 2024

[DAG] SelectionDAG.computeKnownBits - add NSW/NUW flags support to ISD::SHL handling #89877

[DAG] SelectionDAG.computeKnownBits - add NSW/NUW flags support to ISD::SHL handling #89877

Uh oh!

Conversation

zxc12523 commented Apr 24, 2024

Uh oh!

github-actions bot commented Apr 24, 2024

Uh oh!

llvmbot commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 2, 2024

Uh oh!

Uh oh!

llvmbot commented Apr 24, 2024 •

edited

Loading

github-actions bot commented Apr 26, 2024 •

edited

Loading