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

[DAG] Add freeze(assertext(x)) -> assertext(x) folds #94491

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[DAG] Add freeze(assertext(x)) -> assertext(x) folds #94491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e292118
[DAG] Add freeze(assertext(x)) -> assertext(x) folds
bjope Jun 5, 2024
fc07830
fixup: Move the logic to isGuaranteedNotToBeUndefOrPoison
bjope Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion llvm/include/llvm/CodeGen/ISDOpcodes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@ enum NodeType {
/// been extended, and the second is a value type node indicating the width
/// of the extension.
/// NOTE: In case of the source value (or any vector element value) is
/// poisoned the assertion will not be true for that value.
/// poisoned the assertion will not be true for that value and the
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update the globalisel equivalent documentation to match whatever ends up here for G_ASSERT_SEXT/G_ASSERT_ZEXT?

/// corresponding result value will be poison. If a source value isn't
/// satisfying the condition being asserted (while not being poison), then
/// this is considered as immediate undefined behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to say that violating the assertion triggers IUB we also need to audit existing uses to comply with that. One obvious case that doesn't is fptoi legalization:

llvm-project/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

Line 811 in 3ac6a64

// Assert that the converted value fits in the original type. If it doesn't

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought that the fptoi legalization fulfilled this patch. That either the fptoi result is poison (and then it is still poison after the assert), or the result of fptoi would satisfy the assert. This patch is at least not breaking the regression tests related to #66603 .

But maybe this whole idea isn't working anyway. Not sure how to protect from further simplifications of the code leading up to the assert, that could make it less poisonous. It would for example not be legal to just freeze the FP_TO_SINT introduced by legalization, without making sure that the frozen value fulfil the later assert.
So just defining it as immediate UB is probably a lot more complicated than this.

Now I'm just back into the mega-confused-state. And I can't really see how we possibly can handle the (freeze(assertext)) kind of regressions seen in #84924 . Maybe if we try to move the freeze in the other direction when there is an assert (or at least avoid moving it as aggressively in that direction when it might block optimizations.

AssertSext,
AssertZext,

Expand Down
5 changes: 5 additions & 0 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5176,6 +5176,11 @@ bool SelectionDAG::isGuaranteedNotToBeUndefOrPoison(SDValue Op,
return true;
}

case ISD::AssertSext:
case ISD::AssertZext:
return isGuaranteedNotToBeUndefOrPoison(Op.getOperand(0), DemandedElts,
PoisonOnly, Depth + 1);

// TODO: Search for noundef attributes from library functions.

// TODO: Pointers dereferenced by ISD::LOAD/STORE ops are noundef.
Expand Down
20 changes: 9 additions & 11 deletions llvm/test/CodeGen/RISCV/float-convert.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,26 +966,24 @@ define i64 @fcvt_lu_s_sat(float %a) nounwind {
; RV64I-NEXT: sd ra, 24(sp) # 8-byte Folded Spill
; RV64I-NEXT: sd s0, 16(sp) # 8-byte Folded Spill
; RV64I-NEXT: sd s1, 8(sp) # 8-byte Folded Spill
; RV64I-NEXT: sd s2, 0(sp) # 8-byte Folded Spill
; RV64I-NEXT: mv s0, a0
; RV64I-NEXT: lui a1, 391168
; RV64I-NEXT: addiw a1, a1, -1
; RV64I-NEXT: call __gtsf2
; RV64I-NEXT: sgtz a0, a0
; RV64I-NEXT: neg s1, a0
; RV64I-NEXT: mv a0, s0
; RV64I-NEXT: li a1, 0
; RV64I-NEXT: call __gesf2
; RV64I-NEXT: slti a0, a0, 0
; RV64I-NEXT: addi s2, a0, -1
; RV64I-NEXT: addi s1, a0, -1
; RV64I-NEXT: mv a0, s0
; RV64I-NEXT: call __fixunssfdi
; RV64I-NEXT: and a0, s2, a0
; RV64I-NEXT: or a0, s1, a0
; RV64I-NEXT: and s1, s1, a0
; RV64I-NEXT: lui a1, 391168
; RV64I-NEXT: addiw a1, a1, -1
; RV64I-NEXT: mv a0, s0
; RV64I-NEXT: call __gtsf2
; RV64I-NEXT: sgtz a0, a0
; RV64I-NEXT: neg a0, a0
; RV64I-NEXT: or a0, a0, s1
; RV64I-NEXT: ld ra, 24(sp) # 8-byte Folded Reload
; RV64I-NEXT: ld s0, 16(sp) # 8-byte Folded Reload
; RV64I-NEXT: ld s1, 8(sp) # 8-byte Folded Reload
; RV64I-NEXT: ld s2, 0(sp) # 8-byte Folded Reload
; RV64I-NEXT: addi sp, sp, 32
; RV64I-NEXT: ret
start:
Expand Down
12 changes: 7 additions & 5 deletions llvm/test/CodeGen/RISCV/rv64xtheadbb.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,15 +239,17 @@ define i32 @ctlz_lshr_i32(i32 signext %a) {
; RV64I-NEXT: srliw a0, a0, 1
; RV64I-NEXT: beqz a0, .LBB4_2
; RV64I-NEXT: # %bb.1: # %cond.false
; RV64I-NEXT: srliw a1, a0, 1
; RV64I-NEXT: srli a1, a0, 1
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: srliw a1, a0, 2
; RV64I-NEXT: srli a1, a0, 2
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: srliw a1, a0, 4
; RV64I-NEXT: srli a1, a0, 4
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: srliw a1, a0, 8
; RV64I-NEXT: slli a1, a0, 33
; RV64I-NEXT: srli a1, a1, 41
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: srliw a1, a0, 16
; RV64I-NEXT: slli a1, a0, 33
; RV64I-NEXT: srli a1, a1, 49
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: not a0, a0
; RV64I-NEXT: srli a1, a0, 1
Expand Down
14 changes: 8 additions & 6 deletions llvm/test/CodeGen/RISCV/rv64zbb.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,17 @@ define i32 @ctlz_lshr_i32(i32 signext %a) {
; RV64I-NEXT: srliw a0, a0, 1
; RV64I-NEXT: beqz a0, .LBB4_2
; RV64I-NEXT: # %bb.1: # %cond.false
; RV64I-NEXT: srliw a1, a0, 1
; RV64I-NEXT: srli a1, a0, 1
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: srliw a1, a0, 2
; RV64I-NEXT: srli a1, a0, 2
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: srliw a1, a0, 4
; RV64I-NEXT: srli a1, a0, 4
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: srliw a1, a0, 8
; RV64I-NEXT: slli a1, a0, 33
; RV64I-NEXT: srli a1, a1, 41
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: srliw a1, a0, 16
; RV64I-NEXT: slli a1, a0, 33
; RV64I-NEXT: srli a1, a1, 49
; RV64I-NEXT: or a0, a0, a1
; RV64I-NEXT: not a0, a0
; RV64I-NEXT: srli a1, a0, 1
Expand Down Expand Up @@ -1336,7 +1338,7 @@ define i32 @abs_i32(i32 %x) {
define signext i32 @abs_i32_sext(i32 signext %x) {
; RV64I-LABEL: abs_i32_sext:
; RV64I: # %bb.0:
; RV64I-NEXT: sraiw a1, a0, 31
; RV64I-NEXT: srai a1, a0, 31
; RV64I-NEXT: xor a0, a0, a1
; RV64I-NEXT: subw a0, a0, a1
; RV64I-NEXT: ret
Expand Down
Loading