Skip to content

[clang][x86][bytecode] Replace interp__builtin_parity/clrsb/bitreverse/ffs with static bool interp__builtin_elementwise_int_unaryop callback #162346

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

Merged
merged 10 commits into from
Oct 8, 2025
Merged

[clang][x86][bytecode] Replace interp__builtin_parity/clrsb/bitreverse/ffs with static bool interp__builtin_elementwise_int_unaryop callback #162346

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
26eada5
Replaced callback for parity llvm#160288
donneypr Oct 7, 2025
9d8d082
Replaced callback for clrsb llvm#160288
donneypr Oct 7, 2025
d5e1e63
Replaced callback for bitreverse llvm#160288
donneypr Oct 7, 2025
b08aa33
fixed typo for clrsb V -> Val
donneypr Oct 7, 2025
8d9c86c
Replaced callback for ffs llvm#160288
donneypr Oct 7, 2025
f1b6cdf
Merge branch 'main' into clang/bytecode-unaryop-refactor
donneypr Oct 7, 2025
ac30e63
clang formatted
donneypr Oct 7, 2025
08f30d2
Bytecode: fix rotate; drop accidental ffs lines
donneypr Oct 7, 2025
8089053
removed rotate as it was removed in PR #161924
donneypr Oct 7, 2025
b5d1c9f
Check to see if Val is 0 for ffs llvm#160288
donneypr Oct 8, 2025
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
57 changes: 17 additions & 40 deletions clang/lib/AST/ByteCode/InterpBuiltin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,30 +678,6 @@ static bool interp__builtin_popcount(InterpState &S, CodePtr OpPC,
return true;
}

static bool interp__builtin_parity(InterpState &S, CodePtr OpPC,
const InterpFrame *Frame,
const CallExpr *Call) {
APSInt Val = popToAPSInt(S, Call->getArg(0));
pushInteger(S, Val.popcount() % 2, Call->getType());
return true;
}

static bool interp__builtin_clrsb(InterpState &S, CodePtr OpPC,
const InterpFrame *Frame,
const CallExpr *Call) {
APSInt Val = popToAPSInt(S, Call->getArg(0));
pushInteger(S, Val.getBitWidth() - Val.getSignificantBits(), Call->getType());
return true;
}

static bool interp__builtin_bitreverse(InterpState &S, CodePtr OpPC,
const InterpFrame *Frame,
const CallExpr *Call) {
APSInt Val = popToAPSInt(S, Call->getArg(0));
pushInteger(S, Val.reverseBits(), Call->getType());
return true;
}

static bool interp__builtin_classify_type(InterpState &S, CodePtr OpPC,
const InterpFrame *Frame,
const CallExpr *Call) {
Expand Down Expand Up @@ -736,16 +712,6 @@ static bool interp__builtin_expect(InterpState &S, CodePtr OpPC,
return true;
}

static bool interp__builtin_ffs(InterpState &S, CodePtr OpPC,
const InterpFrame *Frame,
const CallExpr *Call) {
APSInt Value = popToAPSInt(S, Call->getArg(0));

uint64_t N = Value.countr_zero();
pushInteger(S, N == Value.getBitWidth() ? 0 : N + 1, Call->getType());
return true;
}

static bool interp__builtin_addressof(InterpState &S, CodePtr OpPC,
const InterpFrame *Frame,
const CallExpr *Call) {
Expand Down Expand Up @@ -3158,18 +3124,25 @@ bool InterpretBuiltin(InterpState &S, CodePtr OpPC, const CallExpr *Call,
case Builtin::BI__builtin_parity:
case Builtin::BI__builtin_parityl:
case Builtin::BI__builtin_parityll:
return interp__builtin_parity(S, OpPC, Frame, Call);

return interp__builtin_elementwise_int_unaryop(
S, OpPC, Call, [](const APSInt &Val) -> APInt {
return APInt(Val.getBitWidth(), Val.popcount() % 2);
});
case Builtin::BI__builtin_clrsb:
case Builtin::BI__builtin_clrsbl:
case Builtin::BI__builtin_clrsbll:
return interp__builtin_clrsb(S, OpPC, Frame, Call);

return interp__builtin_elementwise_int_unaryop(
S, OpPC, Call, [](const APSInt &Val) -> APInt {
return APInt(Val.getBitWidth(),
Val.getBitWidth() - Val.getSignificantBits());
});
case Builtin::BI__builtin_bitreverse8:
case Builtin::BI__builtin_bitreverse16:
case Builtin::BI__builtin_bitreverse32:
case Builtin::BI__builtin_bitreverse64:
return interp__builtin_bitreverse(S, OpPC, Frame, Call);
return interp__builtin_elementwise_int_unaryop(
S, OpPC, Call,
[](const APSInt &Val) -> APInt { return Val.reverseBits(); });

case Builtin::BI__builtin_classify_type:
return interp__builtin_classify_type(S, OpPC, Frame, Call);
Expand Down Expand Up @@ -3209,7 +3182,11 @@ bool InterpretBuiltin(InterpState &S, CodePtr OpPC, const CallExpr *Call,
case Builtin::BI__builtin_ffs:
case Builtin::BI__builtin_ffsl:
case Builtin::BI__builtin_ffsll:
return interp__builtin_ffs(S, OpPC, Frame, Call);
return interp__builtin_elementwise_int_unaryop(
S, OpPC, Call, [](const APSInt &Val) {
Copy link
Collaborator

@shafik shafik Oct 10, 2025

Choose a reason for hiding this comment

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

Curious, you only left off the trailing return type on this one.

Copy link
Contributor Author

@donneypr donneypr Oct 14, 2025

Choose a reason for hiding this comment

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

Missed it on my commit. This should be fixed, thank you for pointing it out. I can get it fixed with my next PR.

Copy link
Collaborator

@RKSimon RKSimon Oct 14, 2025

Choose a reason for hiding this comment

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

@shafik It doesn't look like any of the interp__builtin_elementwise_int_unaryop calls require a trailing type - why do you want it adding here instead of stripping it off the others?

Copy link
Collaborator

@shafik shafik Oct 14, 2025

Choose a reason for hiding this comment

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

My assumption was that in the Val.reverseBits() case it was not obvious and it was added to the others for uniformity and the fourth was accidently left out. The PR was accepted so this felt like a valid take. I would also be consistent if you removed it from all of them.

@tbaederr how do you feel?

Copy link
Contributor

@tbaederr tbaederr Oct 15, 2025

Choose a reason for hiding this comment

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

I usually add a trailing return type when I write lambdas, but I think that's the exception when looking at the entire code base, so I don't mind either way.

Copy link
Contributor Author

@donneypr donneypr Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Just my novice 2 cents: If we're keeping the trailing return type for some of the functions eg. Val.reverseBits(), I think it would better to add it to all of them for uniformity. I wasn't going to add it initially but a previous PR had the trailing type so I used it to stay consistent.

return APInt(Val.getBitWidth(),
Val.isZero() ? 0u : Val.countTrailingZeros() + 1u);
});

case Builtin::BIaddressof:
case Builtin::BI__addressof:
Expand Down