This is an archive of the discontinued LLVM Phabricator instance.

StructurizeCFG: fix inverting conditions
ClosedPublic

Authored by hakzsam on May 2 2018, 1:25 AM.

Details

Summary

Without this patch, it appears to me that we are selecting
the wrong operand when inverting conditions. In the attached
test, it will select %tmp3 instead of %tmp4. To fix it, just
use 'A' as everywhere.

This fixes a regression introduced by
"[PatternMatch] define m_Not using m_Xor and cst_pred_ty"

Diff Detail

Event Timeline

hakzsam created this revision.May 2 2018, 1:25 AM
arsenm added inline comments.May 2 2018, 5:45 AM
lib/Transforms/Scalar/StructurizeCFG.cpp
385

I think you can omit the dummy variable from m_Value. If not, use a better name like Unused

test/Transforms/StructurizeCFG/invert-condition.ll
2–3

Can you add more comprehensive test checks?

I think for structurizer tests we should just always use update_tests_checks

spatel added inline comments.May 2 2018, 6:34 AM
lib/Transforms/Scalar/StructurizeCFG.cpp
385

I'm not familiar with this pass (note - please upload diffs with full context: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface ).

But the comment suggests that you want to capture and return the 'not' value, so that would be:

Value *NotCondition;
if (match(Condition, m_Not(m_Value(NotCondition))))
  return NotCondition;
test/Transforms/StructurizeCFG/invert-condition.ll
2–3

+1. It would be even better if this test was committed on trunk first, so we can see only the functional difference of fixing the bug.

arsenm added inline comments.May 2 2018, 6:37 AM
lib/Transforms/Scalar/StructurizeCFG.cpp
385

Ah, yes. This should be returning the value so this shouldn't be a dummy variable

hakzsam updated this revision to Diff 144986.May 3 2018, 1:19 AM

Without this patch, it appears to me that we are selecting
the wrong operand when inverting conditions. In the attached
test, it will select %tmp3 instead of %tmp4. To fix it, just
use 'A' as everywhere.

This fixes a regression introduced by
"[PatternMatch] define m_Not using m_Xor and cst_pred_ty"

v2:

  • fix returning the not condition
  • use update_test_check for generating test checks
a.sidorin resigned from this revision.May 3 2018, 1:33 AM

Malformed Herald rule, sorry.

arsenm accepted this revision.May 3 2018, 1:35 AM

LGTM

This revision is now accepted and ready to land.May 3 2018, 1:35 AM
mareko closed this revision.May 15 2018, 2:52 PM
mareko added a subscriber: mareko.

Committed.