Skip to content

JIT: Fix GenTree::IsPow2 functions #127615

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
BoyBaykiller wants to merge 5 commits into
base: main
Choose a base branch
from
+28 −23
Open

JIT: Fix GenTree::IsPow2 functions #127615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
41a89e1
* fix isPow2 functions
BoyBaykiller Apr 30, 2026
5556984
* improve isPow2Abs impl
BoyBaykiller Apr 30, 2026
e330784
* fix failed '(value < maxVal) || ((value & signBitsMask) == signBits…
BoyBaykiller May 1, 2026
c25c451
* use std::abs
BoyBaykiller May 2, 2026
fa4bf53
* use UnsignedIntegralValue in fgMorphUModToAndSub
BoyBaykiller May 2, 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
17 changes: 3 additions & 14 deletions src/coreclr/jit/emitarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2595,20 +2595,9 @@ emitter::code_t emitter::emitInsCode(instruction ins, insFormat fmt)
{
assert(width <= 64);

UINT64 result = ~value;

if (width < 64)
{
// Check that 'value' fits in 'width' bits. Don't consider "sign" bits above width.
UINT64 maxVal = 1ULL << width;
UINT64 lowBitsMask = maxVal - 1;
UINT64 signBitsMask = ~lowBitsMask | (1ULL << (width - 1)); // The high bits must be set, and the top bit
// (sign bit) must be set.
assert((value < maxVal) || ((value & signBitsMask) == signBitsMask));
Copy link
Copy Markdown
Member

@EgorBo EgorBo May 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

why do we remove this assert?

Copy link
Copy Markdown
Member

@EgorBo EgorBo May 2, 2026

Choose a reason for hiding this comment

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

my understanding it checks that we properly truncated/sign-extended input

Copy link
Copy Markdown
Contributor Author

@BoyBaykiller BoyBaykiller May 2, 2026

Choose a reason for hiding this comment

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

After fixing fgMorphUModToAndSub this assert no longer fires. However I am still not sure why it was done in the first place here. Let me know if I should revert this change. @EgorBo

Copy link
Copy Markdown
Contributor Author

@BoyBaykiller BoyBaykiller May 2, 2026

Choose a reason for hiding this comment

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

Like it asserts the bits in value above 'width' are 0. But why? It seems like a unnecessary limitation. My new impl is still fulfilling the contract of the function.


// mask off any extra bits that we got from the complement operation
result &= lowBitsMask;
}
// Mask for zero'ing bits above width.
UINT64 mask = UINT64_MAX >> (64 - width);
UINT64 result = ~value & mask;

return result;
}
Expand Down
9 changes: 9 additions & 0 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19671,6 +19671,15 @@ bool GenTreeIntConCommon::ImmedValCanBeFolded(Compiler* comp, genTreeOps op)
return !ImmedValNeedsReloc(comp) || (op == GT_EQ) || (op == GT_NE);
}

UINT64 GenTreeIntConCommon::UnsignedIntegralValue() const
{
uint64_t mask = (UINT64_MAX >> (64 - (genTypeSize(this) * BITS_PER_BYTE)));

INT64 signExtended = IntegralValue();
UINT64 zeroExtended = signExtended & mask;
return zeroExtended;
}

#if defined(TARGET_AMD64) || defined(TARGET_RISCV64)
// Returns true if this absolute address fits within the base of an addr mode.
// On Amd64 this effectively means, whether an absolute indirect address can
Expand Down
9 changes: 4 additions & 5 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3337,6 +3337,7 @@ struct GenTreeIntConCommon : public GenTree
inline ssize_t IconValue() const;
inline void SetIconValue(ssize_t val);
inline INT64 IntegralValue() const;
UINT64 UnsignedIntegralValue() const;
inline void SetIntegralValue(int64_t value);

template <typename T>
Expand Down Expand Up @@ -3536,7 +3537,7 @@ inline INT64 GenTreeIntConCommon::IntegralValue() const
#ifdef TARGET_64BIT
return LngValue();
#else
return OperIs(GT_CNS_LNG) ? LngValue() : (INT64)IconValue();
return OperIs(GT_CNS_LNG) ? LngValue() : static_cast<INT64>(IconValue());
#endif // TARGET_64BIT
}

Expand Down Expand Up @@ -10473,7 +10474,7 @@ inline bool GenTree::IsIntegralConstUnsignedPow2() const
{
if (IsIntegralConst())
{
return isPow2((UINT64)AsIntConCommon()->IntegralValue());
return isPow2(AsIntConCommon()->UnsignedIntegralValue());
}

return false;
Expand All @@ -10491,9 +10492,7 @@ inline bool GenTree::IsIntegralConstAbsPow2() const
{
if (IsIntegralConst())
{
INT64 svalue = AsIntConCommon()->IntegralValue();
size_t value = (svalue == SSIZE_T_MIN) ? static_cast<size_t>(svalue) : static_cast<size_t>(abs(svalue));
return isPow2(value);
return isAbsPow2(AsIntConCommon()->IntegralValue());
}

return false;
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4326,8 +4326,8 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp)
#ifdef TARGET_RISCV64
if (bitOp->IsIntegralConstUnsignedPow2())
{
INT64 bit = bitOp->AsIntConCommon()->IntegralValue();
int log2 = BitOperations::Log2((UINT64)bit);
UINT64 bit = bitOp->AsIntConCommon()->UnsignedIntegralValue();
int log2 = BitOperations::Log2(bit);
bitOp->AsIntConCommon()->SetIntegralValue(log2);
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12094,8 +12094,8 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree)

const var_types type = tree->TypeGet();

const size_t cnsValue = (static_cast<size_t>(tree->gtOp2->AsIntConCommon()->IntegralValue())) - 1;
GenTree* const newTree = gtNewOperNode(GT_AND, type, tree->gtOp1, gtNewIconNodeWithVN(this, cnsValue, type));
const UINT64 mask = tree->gtOp2->AsIntConCommon()->UnsignedIntegralValue() - 1;
GenTree* const newTree = gtNewOperNode(GT_AND, type, tree->gtOp1, gtNewIconNodeWithVN(this, mask, type));

newTree->SetMorphed(this);
DEBUG_DESTROY_NODE(tree->gtOp2);
Expand Down
8 changes: 8 additions & 0 deletions src/coreclr/jit/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ inline bool isPow2(T i)
return (i > 0 && ((i - 1) & i) == 0);
}

// return true if abs(arg) is a power of 2
template <typename T>
inline bool isAbsPow2(T i)
{
static_assert(std::numeric_limits<T>::is_signed);
return (i == std::numeric_limits<T>::min()) || isPow2(std::abs(i));
}

template <typename T>
constexpr bool AreContiguous(T val1, T val2)
{
Expand Down
Loading