From 8bdd54bc8c546d8131a3be593501ce402bad0ef8 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 7 May 2023 18:01:24 +0200 Subject: [PATCH 1/6] Fold "FrozenObjectHandle(REF) + CNS" to a byref constant --- src/coreclr/jit/lower.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index de2cd661d7f25..76fa2bc1dba58 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -6247,6 +6247,22 @@ GenTree* Lowering::LowerAdd(GenTreeOp* node) return next; } + // Fold "FrozenObjectHandle(REF) + CNS" to a byref constant: + // + // * ADD byref + // +--* CNS_INT(h) ref + // \--* CNS_INT long + // + // We can do this earlier but that will need some efforts (e.g. to restore original object from a + // byref constant for optimizations, be careful with "base constant" CSEs, etc). Also, it can't be + // just enabled for AOT, it needs some way to use reloc + offset then. + if (!comp->opts.IsReadyToRun() && op1->IsIconHandle(GTF_ICON_OBJ_HDL) && op2->IsCnsIntOrI()) + { + BlockRange().Remove(op1); + BlockRange().Remove(op2); + node->BashToConst(op1->AsIntCon()->IconValue() + op2->AsIntCon()->IconValue(), TYP_BYREF); + } + #ifdef TARGET_XARCH if (BlockRange().TryGetUse(node, &use)) { From 3e57605c050ffe7c2e9fd457f91132c71e9399a7 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 7 May 2023 18:23:45 +0200 Subject: [PATCH 2/6] Address Single's suggestion --- src/coreclr/jit/lower.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 76fa2bc1dba58..52a9f00ad98cb 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -6256,7 +6256,8 @@ GenTree* Lowering::LowerAdd(GenTreeOp* node) // We can do this earlier but that will need some efforts (e.g. to restore original object from a // byref constant for optimizations, be careful with "base constant" CSEs, etc). Also, it can't be // just enabled for AOT, it needs some way to use reloc + offset then. - if (!comp->opts.IsReadyToRun() && op1->IsIconHandle(GTF_ICON_OBJ_HDL) && op2->IsCnsIntOrI()) + if (op1->IsIconHandle(GTF_ICON_OBJ_HDL) && !op1->AsIntCon()->ImmedValNeedsReloc(comp) && op2->IsCnsIntOrI() && + !op2->IsIconHandle()) { BlockRange().Remove(op1); BlockRange().Remove(op2); From ee2c656a6ea849797372b1f837cedc1abe7d6261 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 7 May 2023 22:53:36 +0200 Subject: [PATCH 3/6] Generalize it --- src/coreclr/jit/lower.cpp | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 52a9f00ad98cb..ccfef7225715f 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -6247,21 +6247,18 @@ GenTree* Lowering::LowerAdd(GenTreeOp* node) return next; } - // Fold "FrozenObjectHandle(REF) + CNS" to a byref constant: - // - // * ADD byref - // +--* CNS_INT(h) ref - // \--* CNS_INT long - // - // We can do this earlier but that will need some efforts (e.g. to restore original object from a - // byref constant for optimizations, be careful with "base constant" CSEs, etc). Also, it can't be - // just enabled for AOT, it needs some way to use reloc + offset then. - if (op1->IsIconHandle(GTF_ICON_OBJ_HDL) && !op1->AsIntCon()->ImmedValNeedsReloc(comp) && op2->IsCnsIntOrI() && - !op2->IsIconHandle()) + // Fold ADD(CNS1, CNS2) we mainly target a very specific pattern - ADD(CNS_INT(ref), CNS_INT) where + // the first icon handle is a frozen object, we could do this folding earlier but that is not trivial + // as we'll have to introduce a way to restore original object from a byref constant for optimizations. + if (op1->IsCnsIntOrI() && op2->IsCnsIntOrI() && + // Make sure both constants don't need relocs. TODO-CQ: we should allow this for AOT too. + // For that we need to guarantee that the new constant will be lowered as the original handle + // with offset in a reloc. + !op1->AsIntCon()->ImmedValNeedsReloc(comp) && !op2->AsIntCon()->ImmedValNeedsReloc(comp)) { BlockRange().Remove(op1); BlockRange().Remove(op2); - node->BashToConst(op1->AsIntCon()->IconValue() + op2->AsIntCon()->IconValue(), TYP_BYREF); + node->BashToConst(op1->AsIntCon()->IconValue() + op2->AsIntCon()->IconValue(), node->TypeGet()); } #ifdef TARGET_XARCH From 153cdbb0f8a5dc42b99bc998e3c5c34a52247377 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 8 May 2023 00:31:19 +0200 Subject: [PATCH 4/6] fix tests --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index ccfef7225715f..d26ab2d1f126a 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -6250,7 +6250,7 @@ GenTree* Lowering::LowerAdd(GenTreeOp* node) // Fold ADD(CNS1, CNS2) we mainly target a very specific pattern - ADD(CNS_INT(ref), CNS_INT) where // the first icon handle is a frozen object, we could do this folding earlier but that is not trivial // as we'll have to introduce a way to restore original object from a byref constant for optimizations. - if (op1->IsCnsIntOrI() && op2->IsCnsIntOrI() && + if (op1->IsCnsIntOrI() && op2->IsCnsIntOrI() && !node->gtOverflow() && node->TypeIs(TYP_I_IMPL, TYP_BYREF) && // Make sure both constants don't need relocs. TODO-CQ: we should allow this for AOT too. // For that we need to guarantee that the new constant will be lowered as the original handle // with offset in a reloc. From 41cebff625488599f2fb9dc30d483a6bda10ff54 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 8 May 2023 00:38:25 +0200 Subject: [PATCH 5/6] check opts --- src/coreclr/jit/lower.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d26ab2d1f126a..04011081177d6 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -6247,15 +6247,15 @@ GenTree* Lowering::LowerAdd(GenTreeOp* node) return next; } - // Fold ADD(CNS1, CNS2) we mainly target a very specific pattern - ADD(CNS_INT(ref), CNS_INT) where - // the first icon handle is a frozen object, we could do this folding earlier but that is not trivial - // as we'll have to introduce a way to restore original object from a byref constant for optimizations. - if (op1->IsCnsIntOrI() && op2->IsCnsIntOrI() && !node->gtOverflow() && node->TypeIs(TYP_I_IMPL, TYP_BYREF) && - // Make sure both constants don't need relocs. TODO-CQ: we should allow this for AOT too. - // For that we need to guarantee that the new constant will be lowered as the original handle - // with offset in a reloc. - !op1->AsIntCon()->ImmedValNeedsReloc(comp) && !op2->AsIntCon()->ImmedValNeedsReloc(comp)) - { + // Fold ADD(CNS1, CNS2). We mainly target a very specific pattern - byref ADD(frozen_handle, cns_offset) + // We could do this folding earlier, but that is not trivial as we'll have to introduce a way to restore + // the original object from a byref constant for optimizations. + if (comp->opts.OptimizationEnabled() && op1->IsCnsIntOrI() && op2->IsCnsIntOrI() && !node->gtOverflow() && + !op1->AsIntCon()->ImmedValNeedsReloc(comp) && !op2->AsIntCon()->ImmedValNeedsReloc(comp) && + node->TypeIs(TYP_I_IMPL, TYP_BYREF)) + { + // TODO-CQ: we should allow this for AOT too. For that we need to guarantee that the new constant + // will be lowered as the original handle with offset in a reloc. BlockRange().Remove(op1); BlockRange().Remove(op2); node->BashToConst(op1->AsIntCon()->IconValue() + op2->AsIntCon()->IconValue(), node->TypeGet()); From 9e5e43b10ae43f6963dc0c593aa58f363f6cde04 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 8 May 2023 19:57:24 +0200 Subject: [PATCH 6/6] clean up --- src/coreclr/jit/lower.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 04011081177d6..2e219ae329891 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -6251,9 +6251,11 @@ GenTree* Lowering::LowerAdd(GenTreeOp* node) // We could do this folding earlier, but that is not trivial as we'll have to introduce a way to restore // the original object from a byref constant for optimizations. if (comp->opts.OptimizationEnabled() && op1->IsCnsIntOrI() && op2->IsCnsIntOrI() && !node->gtOverflow() && - !op1->AsIntCon()->ImmedValNeedsReloc(comp) && !op2->AsIntCon()->ImmedValNeedsReloc(comp) && - node->TypeIs(TYP_I_IMPL, TYP_BYREF)) + (op1->IsIconHandle(GTF_ICON_OBJ_HDL) || op2->IsIconHandle(GTF_ICON_OBJ_HDL)) && + !op1->AsIntCon()->ImmedValNeedsReloc(comp) && !op2->AsIntCon()->ImmedValNeedsReloc(comp)) { + assert(node->TypeIs(TYP_I_IMPL, TYP_BYREF)); + // TODO-CQ: we should allow this for AOT too. For that we need to guarantee that the new constant // will be lowered as the original handle with offset in a reloc. BlockRange().Remove(op1);