Skip to content

[RISCV] Inhibit DAG folding shl through zext.w pattern with zba #91626

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

Closed
wants to merge 2 commits into from
Closed

[RISCV] Inhibit DAG folding shl through zext.w pattern with zba #91626

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
2221121
[RISCV] Inhibit DAG folding shl through zext.w pattern with zba
preames May 8, 2024
54fc5f6
Clang format
preames May 9, 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
8 changes: 8 additions & 0 deletions llvm/lib/Target/RISCV/RISCVISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17141,6 +17141,14 @@ bool RISCVTargetLowering::isDesirableToCommuteWithShift(
return false;
}
}

// Don't break slli.uw patterns.
if (Subtarget.hasStdExtZba() && Ty.isScalarInteger() &&
N->getOpcode() == ISD::SHL && N0.getOpcode() == ISD::AND &&
isa<ConstantSDNode>(N0.getOperand(1)) &&
N0.getConstantOperandVal(1) == UINT64_C(0xffffffff))
return false;

return true;
}

Expand Down
6 changes: 2 additions & 4 deletions llvm/test/CodeGen/RISCV/rv64zba.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2866,8 +2866,7 @@ define ptr @gep_lshr_i32(ptr %0, i64 %1) {
;
; RV64ZBA-LABEL: gep_lshr_i32:
; RV64ZBA: # %bb.0: # %entry
; RV64ZBA-NEXT: slli a1, a1, 2
; RV64ZBA-NEXT: srli a1, a1, 4
; RV64ZBA-NEXT: srli a1, a1, 2
; RV64ZBA-NEXT: slli.uw a1, a1, 4
; RV64ZBA-NEXT: sh2add a1, a1, a1
; RV64ZBA-NEXT: add a0, a0, a1
Expand All @@ -2891,8 +2890,7 @@ define i64 @srli_slliw(i64 %1) {
;
; RV64ZBA-LABEL: srli_slliw:
; RV64ZBA: # %bb.0: # %entry
; RV64ZBA-NEXT: slli a0, a0, 2
; RV64ZBA-NEXT: srli a0, a0, 4
; RV64ZBA-NEXT: srli a0, a0, 2
; RV64ZBA-NEXT: slli.uw a0, a0, 4
; RV64ZBA-NEXT: ret
entry:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this IR non-canonical per InstCombine

Copy link
Member

Choose a reason for hiding this comment

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

The original test case is canonical: https://godbolt.org/z/ee3YPfY4s

define ptr @test(ptr %0, i64 %1) {
entry:
  %2 = lshr exact i64 %1, 2
  %3 = and i64 %2, 4294967295
  %4 = getelementptr inbounds i8, ptr %0, i64 600
  %5 = getelementptr [80 x i8], ptr %4, i64 %3
  ret ptr %5
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes with a GEP its canonical, but with a shl it isn'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.

Yeah, this was excessive reduction apparently.

The fact instcombine prefers the opposite form does hint that we should maybe (also?) do the late match. I was really hoping not to have to write that code...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just posted it. We had the majority of the code already. Is there a 3 shift version of this we should do without Zba?

Expand Down