Skip to content

Commit

Permalink
Fix infinite loop between strength reduction and DCE.
Browse files Browse the repository at this point in the history
0-bit params could lead to strength reduction replacing it with a literal and DCE removing it, which causes an infinite loop when run to fixed point.

PiperOrigin-RevId: 681593175
  • Loading branch information
grebe authored and copybara-github committed Oct 2, 2024
1 parent e827137 commit ce2089f
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 2 deletions.
6 changes: 5 additions & 1 deletion xls/passes/strength_reduction_pass.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,11 @@ absl::StatusOr<bool> StrengthReduceNode(
return false;
}

if (NarrowingEnabled(opt_level) && !node->Is<Literal>() &&
if (NarrowingEnabled(opt_level) &&
// Don't replace unused nodes. We don't want to add nodes when they will
// get DCEd later. This can lead to an infinite loop between strength
// reduction and DCE.
!node->users().empty() && !node->Is<Literal>() &&
query_engine.IsFullyKnown(node)) {
VLOG(2) << "Replacing node with its (entirely known) value: " << node
<< " as " << query_engine.KnownValue(node)->ToString();
Expand Down
35 changes: 34 additions & 1 deletion xls/passes/strength_reduction_pass_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ namespace xls {
namespace {

using status_testing::IsOkAndHolds;

using ::testing::_;
using ::testing::Each;
using ::testing::UnorderedElementsAre;
using ::testing::VariantWith;

class StrengthReductionPassTest : public IrTestBase {
Expand Down Expand Up @@ -416,7 +419,7 @@ TEST_F(StrengthReductionPassTest, ArithToSelect) {
ASSERT_THAT(Run(f), IsOkAndHolds(true));
// Actual verification of result is done by semantics test.
EXPECT_THAT(f->return_value()->operands(),
testing::Each(m::Select(m::Eq(), {m::Literal(), m::Literal()})));
Each(m::Select(m::Eq(), {m::Literal(), m::Literal()})));
}

TEST_F(StrengthReductionPassTest, ArithToSelectOnlyWithOneBit) {
Expand Down Expand Up @@ -476,5 +479,35 @@ TEST_F(StrengthReductionPassTest, DoNotPushDownMultipleSelects) {
ASSERT_THAT(Run(f), IsOkAndHolds(false)) << f->DumpIr();
}

TEST_F(StrengthReductionPassTest, ReplaceWidth0Param) {
auto p = CreatePackage();
FunctionBuilder fb(TestName(), p.get());
BValue ret = fb.Or(fb.ZeroExtend(fb.Param("x", p->GetBitsType(0)), 1),
fb.Param("y", p->GetBitsType(1)));
XLS_ASSERT_OK_AND_ASSIGN(Function * f, fb.BuildWithReturnValue(ret));
EXPECT_THAT(f->nodes(), UnorderedElementsAre(
m::Param("x"), m::Param("y"), m::ZeroExt(),
m::Or(m::ZeroExt(m::Param("x")), m::Param("y"))));
ASSERT_THAT(Run(f), IsOkAndHolds(true));
EXPECT_THAT(f->nodes(),
UnorderedElementsAre(m::Param("x"), m::Param("y"), m::Literal(),
m::Or(m::Literal(0), m::Param("y"))));
}

TEST_F(StrengthReductionPassTest, DoNotReplaceUnusedWidth0Param) {
auto p = CreatePackage();
FunctionBuilder fb(TestName(), p.get());
fb.Param("x", p->GetBitsType(0));
XLS_ASSERT_OK_AND_ASSIGN(
Function * f, fb.BuildWithReturnValue(fb.Param("y", p->GetBitsType(1))));
EXPECT_THAT(f->nodes(), UnorderedElementsAre(m::Param("x"), m::Param("y")));
// Normally, the empty param would be replaced with a literal, but since it
// is unused, it doesn't get replaced.
// Replacing unused params with literals can lead to an infinite loop with
// strength reduction adding the literal and DCE removing it.
ASSERT_THAT(Run(f), IsOkAndHolds(false));
EXPECT_THAT(f->nodes(), UnorderedElementsAre(m::Param("x"), m::Param("y")));
}

} // namespace
} // namespace xls

0 comments on commit ce2089f

Please sign in to comment.