Skip to content
Merged
Changes from 1 commit
Commits
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
Next Next commit
JIT: don't forward sub a volatile read tree via copy
The copy may not get the same value as the original.

Closes #62048.
  • Loading branch information
AndyAyersMS committed Dec 1, 2021
commit 06d566f9425050c7b83792fe7724237b54c2764f
55 changes: 55 additions & 0 deletions src/coreclr/jit/redundantbranchopts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume checking for GTF_ORDER_SIDEEFF resulted in too many (avoidable) regressions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, let me check -- doing that would be simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems to work out just as well, so will update.

// 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<HasVolatileRead>
{
public:
enum
{
DoPreOrder = true
};

GenTree* m_read;

HasVolatileRead(Compiler* compiler) : GenTreeVisitor<HasVolatileRead>(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;
}
Expand Down