From 06d566f9425050c7b83792fe7724237b54c2764f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 30 Nov 2021 17:27:21 -0800 Subject: [PATCH 1/3] JIT: don't forward sub a volatile read tree via copy The copy may not get the same value as the original. Closes #62048. --- src/coreclr/jit/redundantbranchopts.cpp | 55 +++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index d00de9901da4dc..f6d24273eeaa94 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1083,6 +1083,61 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) // We going to forward-sub a copy of candidateTree // assert(!sideEffect); + + // Note the candidateTree's result may still be live. + // + // We should only duplicate candidateTree tree if we're sure the copy + // will produce the same result. In particular if the tree contains + // volatile reads, it may not evaluate the same. + // + // Volatile reads are modelled as side effects in VN but not in the IR. + // Ideally perhaps we could leverage VN to detect when it would be unsafe + // to copy the tree, but it's not possible to do that today. + // + class HasVolatileRead final : public GenTreeVisitor + { + public: + enum + { + DoPreOrder = true + }; + + GenTree* m_read; + + HasVolatileRead(Compiler* compiler) : GenTreeVisitor(compiler), m_read(nullptr) + { + } + + Compiler::fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* const node = *use; + + switch (node->OperGet()) + { + case GT_IND: + case GT_BLK: + case GT_OBJ: + case GT_CLS_VAR: + case GT_FIELD: + if ((node->gtFlags & GTF_IND_VOLATILE) != 0) + { + m_read = node; + return WALK_ABORT; + } + default: + return WALK_CONTINUE; + } + } + }; + + HasVolatileRead v(this); + v.WalkTree(&candidateTree, nullptr); + if (v.m_read != nullptr) + { + JITDUMP(" -- prev tree contains volatile read [%06u], sorry\n", dspTreeID(v.m_read)); + return false; + } + substituteTree = gtCloneExpr(candidateTree); usedCopy = true; } From c4267c7e082e3eb42418ae45d6e1bc1129b4686b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 1 Dec 2021 09:40:50 -0800 Subject: [PATCH 2/3] Revert "JIT: don't forward sub a volatile read tree via copy" This reverts commit 06d566f9425050c7b83792fe7724237b54c2764f. --- src/coreclr/jit/redundantbranchopts.cpp | 55 ------------------------- 1 file changed, 55 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index f6d24273eeaa94..d00de9901da4dc 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1083,61 +1083,6 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) // We going to forward-sub a copy of candidateTree // assert(!sideEffect); - - // Note the candidateTree's result may still be live. - // - // We should only duplicate candidateTree tree if we're sure the copy - // will produce the same result. In particular if the tree contains - // volatile reads, it may not evaluate the same. - // - // Volatile reads are modelled as side effects in VN but not in the IR. - // Ideally perhaps we could leverage VN to detect when it would be unsafe - // to copy the tree, but it's not possible to do that today. - // - class HasVolatileRead final : public GenTreeVisitor - { - public: - enum - { - DoPreOrder = true - }; - - GenTree* m_read; - - HasVolatileRead(Compiler* compiler) : GenTreeVisitor(compiler), m_read(nullptr) - { - } - - Compiler::fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) - { - GenTree* const node = *use; - - switch (node->OperGet()) - { - case GT_IND: - case GT_BLK: - case GT_OBJ: - case GT_CLS_VAR: - case GT_FIELD: - if ((node->gtFlags & GTF_IND_VOLATILE) != 0) - { - m_read = node; - return WALK_ABORT; - } - default: - return WALK_CONTINUE; - } - } - }; - - HasVolatileRead v(this); - v.WalkTree(&candidateTree, nullptr); - if (v.m_read != nullptr) - { - JITDUMP(" -- prev tree contains volatile read [%06u], sorry\n", dspTreeID(v.m_read)); - return false; - } - substituteTree = gtCloneExpr(candidateTree); usedCopy = true; } From 2b1ebbee95c182b512e1abc4d1afd4cbdffa1dfc Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 1 Dec 2021 09:39:37 -0800 Subject: [PATCH 3/3] Use GTF_ORDER_SIDEEFF instead. --- src/coreclr/jit/redundantbranchopts.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index d00de9901da4dc..d8e4f97e68cf82 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -907,13 +907,13 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) continue; } - // If prevTree has side effects other than GTF_EXCEPT or GTF_ASG, bail, + // If prevTree has side effects, bail, // unless it is in the immediately preceeding statement. // // (we'll later show that any exception must come from the RHS as the LHS // will be a simple local). // - if ((prevTree->gtFlags & GTF_SIDE_EFFECT) != (prevTree->gtFlags & (GTF_EXCEPT | GTF_ASG))) + if ((prevTree->gtFlags & (GTF_CALL | GTF_ORDER_SIDEEFF)) != 0) { if (prevStmt->GetNextStmt() != stmt) {