Skip to content

[AMDGPU] Added min operation for MCExprs #199746

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

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

[AMDGPU] Added min operation for MCExprs #199746

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
983d3a5
Added min operation for AMDGPUMCExprs
May 26, 2026
501978f
Fix Formating
May 27, 2026
5cf5536
Changing umin and umax to smin and smax for known bits
May 29, 2026
1b802ab
Added unit tests for AMDGPUExpr folding for negative numbers
Jun 1, 2026
eb67b34
fix formating
Jun 1, 2026
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
3 changes: 3 additions & 0 deletions llvm/docs/AMDGPUUsage.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2568,6 +2568,9 @@ As part of the AMDGPU MC layer, AMDGPU provides the following target-specific
``max(arg, ...)`` 1 or more Variadic signed operation that returns the maximum
value of all its arguments.

``min(arg, ...)`` 1 or more Variadic signed operation that returns the minimum
value of all its arguments

``or(arg, ...)`` 1 or more Variadic signed operation that returns the bitwise-or
result of all its arguments.

Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9344,7 +9344,8 @@ void AMDGPUAsmParser::onBeginOfFile() {
/// Parse AMDGPU specific expressions.
///
/// expr ::= or(expr, ...) |
/// max(expr, ...)
/// max(expr, ...) |
/// min(expr, ...)
///
bool AMDGPUAsmParser::parsePrimaryExpr(const MCExpr *&Res, SMLoc &EndLoc) {
using AGVK = AMDGPUMCExpr::VariantKind;
Expand All @@ -9353,6 +9354,7 @@ bool AMDGPUAsmParser::parsePrimaryExpr(const MCExpr *&Res, SMLoc &EndLoc) {
StringRef TokenId = getTokenStr();
AGVK VK = StringSwitch<AGVK>(TokenId)
.Case("max", AGVK::AGVK_Max)
.Case("min", AGVK::AGVK_Min)
.Case("or", AGVK::AGVK_Or)
.Case("extrasgprs", AGVK::AGVK_ExtraSGPRs)
.Case("totalnumvgprs", AGVK::AGVK_TotalNumVGPRs)
Expand Down
15 changes: 15 additions & 0 deletions llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ void AMDGPUMCExpr::printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const {
case AGVK_Max:
OS << "max(";
break;
case AGVK_Min:
OS << "min(";
break;
case AGVK_ExtraSGPRs:
OS << "extrasgprs(";
break;
Expand Down Expand Up @@ -103,6 +106,8 @@ static int64_t op(AMDGPUMCExpr::VariantKind Kind, int64_t Arg1, int64_t Arg2) {
return std::max(Arg1, Arg2);
case AMDGPUMCExpr::AGVK_Or:
return Arg1 | Arg2;
case AMDGPUMCExpr::AGVK_Min:
return std::min(Arg1, Arg2);
}
}

Expand Down Expand Up @@ -499,6 +504,16 @@ static void targetOpKnownBitsMapHelper(const MCExpr *Expr, KnownBitsMap &KBM,
KBM[Expr] = std::move(KB);
return;
}
case AMDGPUMCExpr::VariantKind::AGVK_Min: {
knownBitsMapHelper(AGVK->getSubExpr(0), KBM, Depth + 1);
KnownBits KB = KBM[AGVK->getSubExpr(0)];
for (const MCExpr *Arg : AGVK->getArgs()) {
knownBitsMapHelper(Arg, KBM, Depth + 1);
KB = KnownBits::umin(KB, KBM[Arg]);
Copy link
Copy Markdown
Contributor

@bcahoon bcahoon May 27, 2026

Choose a reason for hiding this comment

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

It's odd that this uses umin, when the definition described in AMGPUUsage.rst says the operator is for signed values. I'm sure this is because Max uses umax, but I'm not sure why that doesn't use smax instead.

Copy link
Copy Markdown
Author

@JoshuaGrindstaff JoshuaGrindstaff May 28, 2026

Choose a reason for hiding this comment

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

We probably will need an explanation from Janek but after looking into it, my guess is that the expressions that we are folding with the KB will always be positive and small enough to not set the most significant bit to 1. So if the sign bit is always zero then there is no practical difference between using smax/smin and umax/umin. These are just used for folding, and the evaluation uses std::min and std::max which both use signed integers.

If this is the reason, it might be good to use smax/smin if there are any expression not within this domain or at the very least to have it consistent with the evaluation step.

Copy link
Copy Markdown
Contributor

@arsenm arsenm May 29, 2026

Choose a reason for hiding this comment

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

Should just make everything signed

Copy link
Copy Markdown
Contributor

@JanekvO JanekvO Jun 1, 2026

Choose a reason for hiding this comment

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

Yes, should be smax/smin

Had a look as to why this didn't show up in tests (apart from our specific resource info use-cases being on unsigned values): the unknown value needs to be a fully resolvable negative (which sounds like a bit of a contradiction). If a symbol is fully known it'll resolve through the evaluateAsAbsolute which correctly uses the signed std::max/min. I think the KnownBits max/min can be triggered here using something along the lines of max(((external_unknown & 0) | 0x8000000000000000), 5) which elides evaluateAsAbsolute for resolving but does end up with KB resolving determining it's a negative value through the bitpattern.

Sorry, a bit of a writeup that just concludes "use smax/smin" 😅
My only ask: could you add tests along the lines of [min|max](((external_unknown & 0) | 0x8000000000000000), 5) that showcase the codepath of KnownBits (partial-)resolving for amdgpu MCExprs? LGTM otherwise

Copy link
Copy Markdown
Author

@JoshuaGrindstaff JoshuaGrindstaff Jun 1, 2026

Choose a reason for hiding this comment

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

@JanekvO Thanks for the write up, it actually helped me out a lot.

I tried to implement your suggestion using lit tests, but it seems the .set won't trigger the function foldAMDGPUMCExpr. I tried putting negative values for the resource info tests but those would fail since there is a check for negative numbers before the folding happens. Because of this, unit tests seemed like the more logical way to go. I confirmed that the tests fail if the code is set to umax/umin.

Since I am not a contributor yet, if this looks good to you, could you merge this PR for me please?

}
KBM[Expr] = std::move(KB);
return;
}
case AMDGPUMCExpr::VariantKind::AGVK_ExtraSGPRs:
case AMDGPUMCExpr::VariantKind::AGVK_TotalNumVGPRs:
case AMDGPUMCExpr::VariantKind::AGVK_AlignTo:
Expand Down
11 changes: 9 additions & 2 deletions llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCExpr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ enum class LitModifier { None, Lit, Lit64 };
/// operations are:
/// - (bitwise) or
/// - max
/// - min
///
/// \note If the 'or'/'max' operations are provided only a single argument, the
/// operation will act as a no-op and simply resolve as the provided argument.
/// \note If the 'or'/'max'/'min' operations are provided only a single
/// argument, the operation will act as a no-op and simply resolve as the
/// provided argument.
///
class AMDGPUMCExpr : public MCTargetExpr {
public:
Expand All @@ -41,6 +43,7 @@ class AMDGPUMCExpr : public MCTargetExpr {
AGVK_InstPrefSize,
AGVK_Lit,
AGVK_Lit64,
AGVK_Min,
};

// Relocation specifiers.
Expand Down Expand Up @@ -85,6 +88,10 @@ class AMDGPUMCExpr : public MCTargetExpr {
MCContext &Ctx) {
return create(VariantKind::AGVK_Max, Args, Ctx);
}
static const AMDGPUMCExpr *createMin(ArrayRef<const MCExpr *> Args,
MCContext &Ctx) {
return create(VariantKind::AGVK_Min, Args, Ctx);
}

static const AMDGPUMCExpr *createExtraSGPRs(const MCExpr *VCCUsed,
const MCExpr *FlatScrUsed,
Expand Down
63 changes: 62 additions & 1 deletion llvm/test/MC/AMDGPU/mcexpr_amd.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
// OBJDUMP-NEXT: 000000000000000a l *ABS* 0000000000000000 max_literals
// OBJDUMP-NEXT: 000000000000000f l *ABS* 0000000000000000 max_with_max_sym
// OBJDUMP-NEXT: 000000000000000f l *ABS* 0000000000000000 max
// OBJDUMP-NEXT: ffffffffffffffff l *ABS* 0000000000000000 neg_one
// OBJDUMP-NEXT: ffffffffffffffff l *ABS* 0000000000000000 max_neg_numbers
// OBJDUMP-NEXT: ffffffffffffffff l *ABS* 0000000000000000 max_neg_number
// OBJDUMP-NEXT: 0000000000000003 l *ABS* 0000000000000000 max_with_subexpr
Expand All @@ -29,6 +28,24 @@
// OBJDUMP-NEXT: 8000000000000000 l *ABS* 0000000000000000 max_expr_one_min
// OBJDUMP-NEXT: 0000000000000003 l *ABS* 0000000000000000 max_expr_two_min
// OBJDUMP-NEXT: 0000000000989680 l *ABS* 0000000000000000 max_expr_three_min
// OBJDUMP-NEXT: 0000000000000001 l *ABS* 0000000000000000 min_expression_all
// OBJDUMP-NEXT: 0000000000000001 l *ABS* 0000000000000000 min_expression_two
// OBJDUMP-NEXT: 0000000000000003 l *ABS* 0000000000000000 min_expression_one
// OBJDUMP-NEXT: 0000000000000001 l *ABS* 0000000000000000 min_literals
// OBJDUMP-NEXT: 0000000000000000 l *ABS* 0000000000000000 min_with_min_sym
// OBJDUMP-NEXT: 0000000000000000 l *ABS* 0000000000000000 min
// OBJDUMP-NEXT: ffffffffffffffff l *ABS* 0000000000000000 neg_one
// OBJDUMP-NEXT: fffffffffffffffb l *ABS* 0000000000000000 min_neg_numbers
// OBJDUMP-NEXT: ffffffffffffffff l *ABS* 0000000000000000 min_neg_number
// OBJDUMP-NEXT: 0000000000000003 l *ABS* 0000000000000000 min_with_subexpr
// OBJDUMP-NEXT: 0000000000000004 l *ABS* 0000000000000000 min_as_subexpr
// OBJDUMP-NEXT: 0000000000000001 l *ABS* 0000000000000000 min_recursive_subexpr
// OBJDUMP-NEXT: 7fffffffffffffff l *ABS* 0000000000000000 min_expr_one_max
// OBJDUMP-NEXT: 0000000000000003 l *ABS* 0000000000000000 min_expr_two_max
// OBJDUMP-NEXT: ffffffffff676980 l *ABS* 0000000000000000 min_expr_three_max
// OBJDUMP-NEXT: 8000000000000000 l *ABS* 0000000000000000 min_expr_one_min
// OBJDUMP-NEXT: 8000000000000000 l *ABS* 0000000000000000 min_expr_two_min
// OBJDUMP-NEXT: 8000000000000000 l *ABS* 0000000000000000 min_expr_three_min
// OBJDUMP-NEXT: 0000000000000007 l *ABS* 0000000000000000 or_expression_all
// OBJDUMP-NEXT: 0000000000000003 l *ABS* 0000000000000000 or_expression_two
// OBJDUMP-NEXT: 0000000000000001 l *ABS* 0000000000000000 or_expression_one
Expand Down Expand Up @@ -97,6 +114,49 @@
.set max_expr_two_min, max(i64_min, three)
.set max_expr_three_min, max(i64_min, three, 10000000)

// ASM: .set min_expression_all, min(1, 2, five, 3, four)
// ASM: .set min_expression_two, 1
// ASM: .set min_expression_one, 3
// ASM: .set min_literals, 1
// ASM: .set min_with_min_sym, min(min, 4, 3, 1, 2)

.set min_expression_all, min(one, two, five, three, four)
.set min_expression_two, min(one, three)
.set min_expression_one, min(three)
.set min_literals, min(1,2,3,4,5,6,7,8,9,10)
.set min_with_min_sym, min(min, 4, 3, one, two)

// ASM: .set min_neg_numbers, -5
// ASM: .set min_neg_number, -1

.set neg_one, -1
.set min_neg_numbers, min(-5, -4, -3, -2, neg_one)
.set min_neg_number, min(neg_one)

// ASM: .set min_with_subexpr, 3
// ASM: .set min_as_subexpr, 1+min(4, 3, five)
// ASM: .set min_recursive_subexpr, min(min(1, four), 3, min_expression_all)

.set min_with_subexpr, min(((one | 3) << 3) / 8)
.set min_as_subexpr, 1 + min(4, 3, five)
.set min_recursive_subexpr, min(min(one, four), three, min_expression_all)

// ASM: .set min_expr_one_max, 9223372036854775807
// ASM: .set min_expr_two_max, 3
// ASM: .set min_expr_three_max, -10000000

.set min_expr_one_max, min(i64_max)
.set min_expr_two_max, min(i64_max, three)
.set min_expr_three_max, min(i64_max, three, -10000000)

// ASM: .set min_expr_one_min, -9223372036854775808
// ASM: .set min_expr_two_min, min(-9223372036854775808, five)
// ASM: .set min_expr_three_min, min(-9223372036854775808, five, 10000000)

.set min_expr_one_min, min(i64_min)
.set min_expr_two_min, min(i64_min, five)
.set min_expr_three_min, min(i64_min, five, 10000000)

// ASM: .set or_expression_all, or(1, 2, five, 3, four)
// ASM: .set or_expression_two, 3
// ASM: .set or_expression_one, 1
Expand Down Expand Up @@ -127,4 +187,5 @@
.set four, 4
.set five, 5
.set max, 0xF
.set min, 0x0
.set or, 0xFF
11 changes: 11 additions & 0 deletions llvm/test/MC/AMDGPU/mcexpr_amd_err.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: empty max expression
// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression

.set min_empty, min()
// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: empty min expression
// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression

.set or_empty, or()
// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: empty or expression
// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
Expand Down Expand Up @@ -40,6 +44,10 @@
// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: unexpected token in or expression
// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression

.set min_expression_one, min(four,five
// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: unexpected token in min expression
// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression

.set max_no_lparen, max four, five)
// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: expected newline

Expand All @@ -49,5 +57,8 @@
.set max_rparen_only, max)
// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: expected newline

.set min_no_lparen, min four, five)
// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: expected newline

.set four, 4
.set five, 5