Skip to content

Commit

Permalink
fully support hoisting of fill-array-data
Browse files Browse the repository at this point in the history
Summary:
`IRInstruction::hash` already expanded the `data` portion of an `IRInstruction`. This makes it so that `operator==` also does a deep equality check over the `data`.

This addresses an internal inconsistency in the branch-prefix hoisting pass.

Reviewed By: wsanville

Differential Revision: D66800414

fbshipit-source-id: 68667e96e2e08b9328f2ef29b5dff45818465c9d
  • Loading branch information
Nikolai Tillmann authored and facebook-github-bot committed Dec 5, 2024
1 parent a902ace commit 7cc52a7
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 4 deletions.
25 changes: 21 additions & 4 deletions libredex/IRInstruction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,30 @@ IRInstruction* IRInstruction::set_data(std::unique_ptr<DexOpcodeData> data) {
// Structural equality of opcodes except branches offsets are ignored
// because they are unknown until we sync back to DexInstructions.
bool IRInstruction::operator==(const IRInstruction& that) const {
bool simple_fields_match =
m_opcode == that.m_opcode &&
m_num_inline_srcs == that.m_num_inline_srcs && m_dest == that.m_dest &&
m_literal == that.m_literal; // just test one member of the union
bool simple_fields_match = m_opcode == that.m_opcode &&
m_num_inline_srcs == that.m_num_inline_srcs &&
m_dest == that.m_dest;
if (!simple_fields_match) {
return false;
}

// Avoid calling opcode::ref(); there's only one instruction that has data.
if (m_opcode == OPCODE_FILL_ARRAY_DATA) {
redex_assert(opcode::ref(m_opcode) == opcode::Ref::Data);
auto size = m_data->data_size();
if (size != that.m_data->data_size() ||
std::memcmp(m_data->data(),
that.m_data->data(),
size * sizeof(uint16_t)) != 0) {
return false;
}
} else {
redex_assert(opcode::ref(m_opcode) != opcode::Ref::Data);
if (m_literal != that.m_literal) { // just test one member of the union
return false;
}
}

// Check the source registers union
if (m_num_inline_srcs <= MAX_NUM_INLINE_SRCS) {
for (auto i = 0; i < m_num_inline_srcs; ++i) {
Expand Down
37 changes: 37 additions & 0 deletions test/unit/BranchPrefixHoistingTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -850,3 +850,40 @@ TEST_F(BranchPrefixHoistingTest, try_catch_in_succ_block) {
)";
test(code_str, expected_str, 0, /* full_validation */ true);
}

TEST_F(BranchPrefixHoistingTest, fill_array_data) {
const auto& code_str = R"(
(
(load-param v0)
(const v1 1)
(new-array v1 "[I")
(move-result-pseudo-object v1)
(if-eqz v0 :true)
(fill-array-data v1 #4 (0))
(fill-array-data v1 #4 (1))
(goto :end)
(:true)
(fill-array-data v1 #4 (0))
(fill-array-data v1 #4 (2))
(:end)
(return-void)
)
)";
const auto& expected_str = R"(
(
(load-param v0)
(const v1 1)
(new-array v1 "[I")
(move-result-pseudo-object v1)
(fill-array-data v1 #4 (0))
(if-eqz v0 :true)
(fill-array-data v1 #4 (1))
(goto :end)
(:true)
(fill-array-data v1 #4 (2))
(:end)
(return-void)
)
)";
test(code_str, expected_str, 1);
}

0 comments on commit 7cc52a7

Please sign in to comment.