Skip to content

Commit b744e1f

Browse files
Fix bad folding (#54722)
* Always sign-extend from int32 in gtFoldExprConst GT_CNS_INT of TYP_INT on 64 hosts has an implicit contract: the value returned by IconValue() must "fit" into 32 bit signed integer, i. e. "static_cast<int>(IconValue()) == IconValue()". gtFoldExprConst was failing to uphold this contract when the target was 32 bit and the host was 64 bit. Fix this by always truncating before calling SetIconValue(). * Add a simple test that reproduces bad codegen
1 parent fe44d58 commit b744e1f

File tree

3 files changed

+41
-8
lines changed

3 files changed

+41
-8
lines changed

src/coreclr/jit/gentree.cpp

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14856,19 +14856,15 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree)
1485614856
JITDUMP("\nFolding operator with constant nodes into a constant:\n");
1485714857
DISPTREE(tree);
1485814858

14859-
#ifdef TARGET_64BIT
14860-
// Some operations are performed as 64 bit instead of 32 bit so the upper 32 bits
14861-
// need to be discarded. Since constant values are stored as ssize_t and the node
14862-
// has TYP_INT the result needs to be sign extended rather than zero extended.
14863-
i1 = INT32(i1);
14864-
#endif // TARGET_64BIT
14865-
1486614859
// Also all conditional folding jumps here since the node hanging from
1486714860
// GT_JTRUE has to be a GT_CNS_INT - value 0 or 1.
1486814861

1486914862
tree->ChangeOperConst(GT_CNS_INT);
1487014863
tree->ChangeType(TYP_INT);
14871-
tree->AsIntCon()->SetIconValue(i1);
14864+
// Some operations are performed as 64 bit instead of 32 bit so the upper 32 bits
14865+
// need to be discarded. Since constant values are stored as ssize_t and the node
14866+
// has TYP_INT the result needs to be sign extended rather than zero extended.
14867+
tree->AsIntCon()->SetIconValue(static_cast<int>(i1));
1487214868
tree->AsIntCon()->gtFieldSeq = fieldSeq;
1487314869
if (vnStore != nullptr)
1487414870
{
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
public class FoldingExtendsInt32On64BitHostsTest
2+
{
3+
// On 64 bit hosts, 32 bit constants are stored as 64 bit signed values.
4+
// gtFoldExpr failed to properly truncate the folded value to 32 bits when
5+
// the host was 64 bit and the target - 32 bit. Thus local assertion prop
6+
// got the "poisoned" value, which lead to silent bad codegen.
7+
8+
public static int Main()
9+
{
10+
var r1 = 31;
11+
// "Poisoned" value.
12+
var s1 = 0b11 << r1;
13+
14+
if (s1 == 0b11 << 31)
15+
{
16+
return 100;
17+
}
18+
19+
// Just so that Roslyn actually uses locals.
20+
Use(s1);
21+
Use(r1);
22+
23+
return -1;
24+
}
25+
26+
private static void Use(int a) { }
27+
}
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<OutputType>Exe</OutputType>
4+
<Optimize>True</Optimize>
5+
<DebugType>None</DebugType>
6+
</PropertyGroup>
7+
<ItemGroup>
8+
<Compile Include="$(MSBuildProjectName).cs" />
9+
</ItemGroup>
10+
</Project>

0 commit comments

Comments
 (0)