Skip to content

Commit

Permalink
Delete TruncateOrSignExtend32 (dotnet#56904)
Browse files Browse the repository at this point in the history
* Introduce SetValueTruncating

As a less error-prone replacement of TruncateOrSignExtend32.
Add a comment header explaining its intended use case.

* Case $1

The code truncated, for unclear reasons (likely a copy & paste mistake),
the wrong node.

* Case $2

* Case $3

The old code was checking for the type of the shift node,
which is what we want. So that SetValueTruncating sees this
type, set it before setting the value itself.

* Delete TruncateOrSignExtend32

Constant integer nodes never have GTF_UNSIGNED set on them.
Additionally, zero-extending a TYP_INT const is incorrect.
  • Loading branch information
SingleAccretion authored Sep 1, 2021
1 parent 629f60e commit c570efd
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 39 deletions.
54 changes: 40 additions & 14 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -3054,6 +3054,9 @@ struct GenTreeIntConCommon : public GenTree
inline void SetIconValue(ssize_t val);
inline INT64 IntegralValue() const;

template <typename T>
inline void SetValueTruncating(T value);

GenTreeIntConCommon(genTreeOps oper, var_types type DEBUGARG(bool largeNode = false))
: GenTree(oper, type DEBUGARG(largeNode))
{
Expand Down Expand Up @@ -3169,20 +3172,6 @@ struct GenTreeIntCon : public GenTreeIntConCommon

void FixupInitBlkValue(var_types asgType);

#ifdef TARGET_64BIT
void TruncateOrSignExtend32()
{
if (gtFlags & GTF_UNSIGNED)
{
gtIconVal = UINT32(gtIconVal);
}
else
{
gtIconVal = INT32(gtIconVal);
}
}
#endif // TARGET_64BIT

#if DEBUGGABLE_GENTREE
GenTreeIntCon() : GenTreeIntConCommon()
{
Expand Down Expand Up @@ -3261,6 +3250,43 @@ inline INT64 GenTreeIntConCommon::IntegralValue() const
#endif // TARGET_64BIT
}

//------------------------------------------------------------------------
// SetValueTruncating: Set the value, truncating to TYP_INT if necessary.
//
// The function will truncate the supplied value to a 32 bit signed
// integer if the node's type is not TYP_LONG, otherwise setting it
// as-is. Note that this function intentionally does not check for
// small types (such nodes are created in lowering) for TP reasons.
//
// This function is intended to be used where its truncating behavior is
// desirable. One example is folding of ADD(CNS_INT, CNS_INT) performed in
// wider integers, which is typical when compiling on 64 bit hosts, as
// most aritmetic is done in ssize_t's aka int64_t's in that case, while
// the node itself can be of a narrower type.
//
// Arguments:
// value - Value to set, truncating to TYP_INT if the node is not of TYP_LONG
//
// Notes:
// This function is templated so that it works well with compiler warnings of
// the form "Operation may overflow before being assigned to a wider type", in
// case "value" is of type ssize_t, which is common.
//
template <typename T>
inline void GenTreeIntConCommon::SetValueTruncating(T value)
{
static_assert_no_msg((std::is_same<T, int32_t>::value || std::is_same<T, int64_t>::value));

if (TypeIs(TYP_LONG))
{
SetLngValue(value);
}
else
{
SetIconValue(static_cast<int32_t>(value));
}
}

/* gtDblCon -- double constant (GT_CNS_DBL) */

struct GenTreeDblCon : public GenTree
Expand Down
31 changes: 6 additions & 25 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12494,14 +12494,9 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
{
cns1 = op1->AsOp()->gtOp2;
cns2 = op2->AsOp()->gtOp2;
cns1->AsIntCon()->gtIconVal += cns2->AsIntCon()->gtIconVal;
#ifdef TARGET_64BIT
if (cns1->TypeGet() == TYP_INT)
{
// we need to properly re-sign-extend or truncate after adding two int constants above
cns1->AsIntCon()->TruncateOrSignExtend32();
}
#endif // TARGET_64BIT

ssize_t value = cns1->AsIntCon()->IconValue() + cns2->AsIntCon()->IconValue();
cns1->AsIntCon()->SetValueTruncating(value);

tree->AsOp()->gtOp2 = cns1;
DEBUG_DESTROY_NODE(cns2);
Expand Down Expand Up @@ -14124,18 +14119,11 @@ GenTree* Compiler::fgMorphSmpOpOptional(GenTreeOp* tree)
oper = GT_ADD;
tree->ChangeOper(oper);

op2->AsIntCon()->gtIconVal = iadd * imul;
op2->AsIntCon()->SetValueTruncating(iadd * imul);

op1->ChangeOper(GT_MUL);

add->AsIntCon()->gtIconVal = imul;
#ifdef TARGET_64BIT
if (add->gtType == TYP_INT)
{
// we need to properly re-sign-extend or truncate after multiplying two int constants above
add->AsIntCon()->TruncateOrSignExtend32();
}
#endif // TARGET_64BIT
add->AsIntCon()->SetIconValue(imul);
}
}

Expand Down Expand Up @@ -14175,17 +14163,10 @@ GenTree* Compiler::fgMorphSmpOpOptional(GenTreeOp* tree)
/* Change "(val + iadd) << ishf" into "(val<<ishf + iadd<<ishf)" */

tree->ChangeOper(GT_ADD);
ssize_t result = iadd << ishf;
op2->AsIntConCommon()->SetIconValue(result);
#ifdef TARGET_64BIT
if (op1->gtType == TYP_INT)
{
op2->AsIntCon()->TruncateOrSignExtend32();
}
#endif // TARGET_64BIT

// we are reusing the shift amount node here, but the type we want is that of the shift result
op2->gtType = op1->gtType;
op2->AsIntConCommon()->SetValueTruncating(iadd << ishf);

if (cns->gtOper == GT_CNS_INT && cns->AsIntCon()->gtFieldSeq != nullptr &&
cns->AsIntCon()->gtFieldSeq->IsConstantIndexFieldSeq())
Expand Down

0 comments on commit c570efd

Please sign in to comment.