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

[InstCombine] Convert or concat to fshl if opposite or concat exists #68502

Merged
merged 7 commits into from
Nov 20, 2023
Merged

[InstCombine] Convert or concat to fshl if opposite or concat exists #68502

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5b3b1bb
[InstCombine] Refactor matchFunnelShift to allow more pattern (NFC)
HaohaiWen Oct 7, 2023
68ab662
[InstCombine] Convert or concat to fshl if opposite or concat exists
HaohaiWen Oct 7, 2023
aca9aa3
Merge remote-tracking branch 'origin/main' into concat
HaohaiWen Oct 19, 2023
b7810a1
Support padding zero between high and low.
HaohaiWen Oct 20, 2023
116e529
Simplify code. Add more tests
HaohaiWen Oct 23, 2023
c882da8
Fix nuw flag
HaohaiWen Oct 24, 2023
fb1af49
Merge remote-tracking branch 'origin/main' into concat
HaohaiWen Nov 15, 2023
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
59 changes: 57 additions & 2 deletions llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2735,7 +2735,8 @@ Instruction *InstCombinerImpl::matchBSwapOrBitReverse(Instruction &I,
}

/// Match UB-safe variants of the funnel shift intrinsic.
static Instruction *matchFunnelShift(Instruction &Or, InstCombinerImpl &IC) {
static Instruction *matchFunnelShift(Instruction &Or, InstCombinerImpl &IC,
const DominatorTree &DT) {
// TODO: Can we reduce the code duplication between this and the related
// rotate matching code under visitSelect and visitTrunc?
unsigned Width = Or.getType()->getScalarSizeInBits();
Expand Down Expand Up @@ -2840,6 +2841,60 @@ static Instruction *matchFunnelShift(Instruction &Or, InstCombinerImpl &IC) {
return nullptr;

FShiftArgs = {ShVal0, ShVal1, ShAmt};
} else if (isa<ZExtInst>(Or0) || isa<ZExtInst>(Or1)) {
// If there are two 'or' instructions concat variables in opposite order:
//
// Slot1 and Slot2 are all zero bits.
// | Slot1 | Low | Slot2 | High |
// LowHigh = or (shl (zext Low), ZextLowShlAmt), (zext High)
// | Slot2 | High | Slot1 | Low |
// HighLow = or (shl (zext High), ZextHighShlAmt), (zext Low)
//
// the latter 'or' can be safely convert to
// -> HighLow = fshl LowHigh, LowHigh, ZextHighShlAmt
// if ZextLowShlAmt + ZextHighShlAmt == Width.
if (!isa<ZExtInst>(Or1))
std::swap(Or0, Or1);

Value *High, *ZextHigh, *Low;
const APInt *ZextHighShlAmt;
if (!match(Or0,
m_OneUse(m_Shl(m_Value(ZextHigh), m_APInt(ZextHighShlAmt)))))
return nullptr;

if (!match(Or1, m_ZExt(m_Value(Low))) ||
!match(ZextHigh, m_ZExt(m_Value(High))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you actually need the shifted value to be zext? I.e if you have (shl (zext i24 to i32), 8) that is bitwise equivilent to (shl i32, 8)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we only match (shl i32, 8), we can't guarantee it's reverse concat since we don't know if the most significant 8bit in i32 is zero.

%zext.x = zext i8 %x to i32 
%slx = shl nuw i32 %zext.x, 24 
%zext.y = zext i24 %y to i32 
%xy = or i32 %zext.y, %slx        #[x[7:0], y[23:0]]
%sly = shl nuw i32 %zext.y, 8 
%yx = or i32 %zext.x, %sly        #[y[23:0], x[7:0]]

If not match zext:

%zext.x = zext i8 %x to i32 
%slx = shl nuw i32 %zext.x, 24 
%xy = or i32 %y, %slx              #[unknown, y[23,0]]             y[31:24] may not be zero.
%sly = shl nuw i32 %y, 8           #[y[23:0], 0,0,0,0,0,0,0,0]
%yx = or i32 %zext.x, %sly         #[y[23:0], x[7:0]]

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I see, although could handle masking and. Really its just a knownbits check though.

Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to check zext. An example without zext: https://alive2.llvm.org/ce/z/MytePb

return nullptr;

unsigned HighSize = High->getType()->getScalarSizeInBits();
unsigned LowSize = Low->getType()->getScalarSizeInBits();
// Make sure High does not overlap with Low and most significant bits of
// High aren't shifted out.
if (ZextHighShlAmt->ult(LowSize) || ZextHighShlAmt->ugt(Width - HighSize))
Copy link
Contributor

Choose a reason for hiding this comment

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

Think you are missing a negative test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return nullptr;

for (User *U : ZextHigh->users()) {
Value *X, *Y;
if (!match(U, m_Or(m_Value(X), m_Value(Y))))
continue;

if (!isa<ZExtInst>(Y))
std::swap(X, Y);

const APInt *ZextLowShlAmt;
if (!match(X, m_Shl(m_Specific(Or1), m_APInt(ZextLowShlAmt))) ||
!match(Y, m_Specific(ZextHigh)) || !DT.dominates(U, &Or))
continue;

// Make sure Low does not overlap with High and most significant bits of
// Low aren't shifted out and we can rotate shift LowHigh to HighLow.
if (ZextLowShlAmt->ult(HighSize) || ZextLowShlAmt->ugt(Width - LowSize) ||
*ZextLowShlAmt + *ZextHighShlAmt != Width)
Copy link
Contributor

Choose a reason for hiding this comment

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

Think you are missing negative test for this (also for non-dominating).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, non-dominating was already tested:
We checked:
yx = fshl xy, xy,
When instcombine handle first or (combine for xy), it can meet all check expect dominance.
If we don't check dominance, if would be convert to xy = fshl yx, yx which is absolutely wrong.

continue;

FShiftArgs = {U, U, ConstantInt::get(Or0->getType(), *ZextHighShlAmt)};
break;
}
HaohaiWen marked this conversation as resolved.
Show resolved Hide resolved
}

if (FShiftArgs.empty())
Expand Down Expand Up @@ -3341,7 +3396,7 @@ Instruction *InstCombinerImpl::visitOr(BinaryOperator &I) {
/*MatchBitReversals*/ true))
return BitOp;

if (Instruction *Funnel = matchFunnelShift(I, *this))
if (Instruction *Funnel = matchFunnelShift(I, *this, DT))
return Funnel;

if (Instruction *Concat = matchOrConcat(I, Builder))
Expand Down
100 changes: 100 additions & 0 deletions llvm/test/Transforms/InstCombine/funnel.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,106 @@ define <2 x i64> @fshl_select_vector(<2 x i64> %x, <2 x i64> %y, <2 x i64> %sham
ret <2 x i64> %r
}

; Convert 'or concat' to fshl if opposite 'or concat' exists.

define i32 @fshl_concat_i8_i24(i8 %x, i24 %y, ptr %addr) {
; CHECK-LABEL: @fshl_concat_i8_i24(
; CHECK-NEXT: [[ZEXT_X:%.*]] = zext i8 [[X:%.*]] to i32
; CHECK-NEXT: [[SLX:%.*]] = shl nuw i32 [[ZEXT_X]], 24
; CHECK-NEXT: [[ZEXT_Y:%.*]] = zext i24 [[Y:%.*]] to i32
; CHECK-NEXT: [[XY:%.*]] = or i32 [[SLX]], [[ZEXT_Y]]
; CHECK-NEXT: store i32 [[XY]], ptr [[ADDR:%.*]], align 4
; CHECK-NEXT: [[YX:%.*]] = call i32 @llvm.fshl.i32(i32 [[XY]], i32 [[XY]], i32 8)
; CHECK-NEXT: ret i32 [[YX]]
;
%zext.x = zext i8 %x to i32
%slx = shl nuw i32 %zext.x, 24
%zext.y = zext i24 %y to i32
%xy = or i32 %zext.y, %slx
store i32 %xy, ptr %addr, align 4
%sly = shl nuw i32 %zext.y, 8
%yx = or i32 %zext.x, %sly
ret i32 %yx
}

define i32 @fshl_concat_i8_i8(i8 %x, i8 %y, ptr %addr) {
; CHECK-LABEL: @fshl_concat_i8_i8(
; CHECK-NEXT: [[ZEXT_X:%.*]] = zext i8 [[X:%.*]] to i32
; CHECK-NEXT: [[SLX:%.*]] = shl nuw nsw i32 [[ZEXT_X]], 13
; CHECK-NEXT: [[ZEXT_Y:%.*]] = zext i8 [[Y:%.*]] to i32
; CHECK-NEXT: [[XY:%.*]] = or i32 [[SLX]], [[ZEXT_Y]]
; CHECK-NEXT: store i32 [[XY]], ptr [[ADDR:%.*]], align 4
; CHECK-NEXT: [[YX:%.*]] = call i32 @llvm.fshl.i32(i32 [[XY]], i32 [[XY]], i32 19)
; CHECK-NEXT: ret i32 [[YX]]
;
%zext.x = zext i8 %x to i32
%slx = shl nuw i32 %zext.x, 13
%zext.y = zext i8 %y to i32
%xy = or i32 %zext.y, %slx
store i32 %xy, ptr %addr, align 4
%sly = shl nuw i32 %zext.y, 19
%yx = or i32 %zext.x, %sly
ret i32 %yx
}

define i32 @fshl_concat_i16_i16_overlap_drop(i16 %x, i16 %y, ptr %addr) {
; CHECK-LABEL: @fshl_concat_i16_i16_overlap_drop(
; CHECK-NEXT: [[ZEXT_X:%.*]] = zext i16 [[X:%.*]] to i32
; CHECK-NEXT: [[SLX:%.*]] = shl nuw i32 [[ZEXT_X]], 17
; CHECK-NEXT: [[ZEXT_Y:%.*]] = zext i16 [[Y:%.*]] to i32
; CHECK-NEXT: [[XY:%.*]] = or i32 [[SLX]], [[ZEXT_Y]]
; CHECK-NEXT: store i32 [[XY]], ptr [[ADDR:%.*]], align 4
; CHECK-NEXT: [[SLY:%.*]] = shl nuw nsw i32 [[ZEXT_Y]], 15
; CHECK-NEXT: [[YX:%.*]] = or i32 [[SLY]], [[ZEXT_X]]
; CHECK-NEXT: ret i32 [[YX]]
;
%zext.x = zext i16 %x to i32
%slx = shl nuw i32 %zext.x, 17
%zext.y = zext i16 %y to i32
%xy = or i32 %zext.y, %slx
store i32 %xy, ptr %addr, align 4
%sly = shl nuw i32 %zext.y, 15
%yx = or i32 %zext.x, %sly
ret i32 %yx
}

define i32 @fshl_concat_unknown_source(i32 %zext.x, i32 %zext.y, ptr %addr) {
; CHECK-LABEL: @fshl_concat_unknown_source(
; CHECK-NEXT: [[SLX:%.*]] = shl nuw i32 [[ZEXT_X:%.*]], 16
; CHECK-NEXT: [[XY:%.*]] = or i32 [[SLX]], [[ZEXT_Y:%.*]]
; CHECK-NEXT: store i32 [[XY]], ptr [[ADDR:%.*]], align 4
; CHECK-NEXT: [[SLY:%.*]] = shl nuw i32 [[ZEXT_Y]], 16
; CHECK-NEXT: [[YX:%.*]] = or i32 [[SLY]], [[ZEXT_X]]
; CHECK-NEXT: ret i32 [[YX]]
;
%slx = shl nuw i32 %zext.x, 16
%xy = or i32 %zext.y, %slx
store i32 %xy, ptr %addr, align 4
%sly = shl nuw i32 %zext.y, 16
%yx = or i32 %zext.x, %sly
ret i32 %yx
}

define <2 x i32> @fshl_concat_vector(<2 x i8> %x, <2 x i24> %y, ptr %addr) {
; CHECK-LABEL: @fshl_concat_vector(
; CHECK-NEXT: [[ZEXT_X:%.*]] = zext <2 x i8> [[X:%.*]] to <2 x i32>
; CHECK-NEXT: [[SLX:%.*]] = shl nuw <2 x i32> [[ZEXT_X]], <i32 24, i32 24>
; CHECK-NEXT: [[ZEXT_Y:%.*]] = zext <2 x i24> [[Y:%.*]] to <2 x i32>
; CHECK-NEXT: [[XY:%.*]] = or <2 x i32> [[SLX]], [[ZEXT_Y]]
; CHECK-NEXT: store <2 x i32> [[XY]], ptr [[ADDR:%.*]], align 4
; CHECK-NEXT: [[YX:%.*]] = call <2 x i32> @llvm.fshl.v2i32(<2 x i32> [[XY]], <2 x i32> [[XY]], <2 x i32> <i32 8, i32 8>)
; CHECK-NEXT: ret <2 x i32> [[YX]]
;
%zext.x = zext <2 x i8> %x to <2 x i32>
%slx = shl nuw <2 x i32> %zext.x, <i32 24, i32 24>
%zext.y = zext <2 x i24> %y to <2 x i32>
%xy = or <2 x i32> %slx, %zext.y
store <2 x i32> %xy, ptr %addr, align 4
%sly = shl nuw <2 x i32> %zext.y, <i32 8, i32 8>
%yx = or <2 x i32> %sly, %zext.x
ret <2 x i32> %yx
}
HaohaiWen marked this conversation as resolved.
Show resolved Hide resolved

; Negative test - an oversized shift in the narrow type would produce the wrong value.

define i8 @unmasked_shlop_unmasked_shift_amount(i32 %x, i32 %y, i32 %shamt) {
Expand Down
Loading