StructurizeCFG: fix inverting conditions

[hakzsam]

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"

[arsenm]

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]

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]

lib/Transforms/Scalar/StructurizeCFG.cpp

385

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

[hakzsam]

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]

Malformed Herald rule, sorry.

[arsenm]

LGTM

[mareko]

Committed.

lib/Transforms/Scalar/StructurizeCFG.cpp

 
 /// \brief Invert the given condition
 Value *StructurizeCFG::invert(Value *Condition) {
+  Value *A;
+
   // First: Check if it's a constant
   if (Constant *C = dyn_cast<Constant>(Condition))
     return ConstantExpr::getNot(C);
 
   // Second: If the condition is already inverted, return the original value
-  if (match(Condition, m_Not(m_Value(Condition))))
+  if (match(Condition, m_Not(m_Value(A))))
     return Condition;
 
   if (Instruction *Inst = dyn_cast<Instruction>(Condition)) {

test/Transforms/StructurizeCFG/invert-condition.ll

+; RUN: opt -S -structurizecfg %s | FileCheck %s
+
+; CHECK: %0 = xor i1 %tmp4, true
+define void @invert_condition(i1 %arg) {
+bb:
+  %tmp = load volatile float, float addrspace(1)* undef
+  %tmp1 = load volatile float, float addrspace(1)* undef
+  br label %bb2
+
+bb2:                                              ; preds = %bb2, %bb
+  %tmp3 = fcmp oge float %tmp, %tmp1
+  %tmp4 = xor i1 %arg, %tmp3
+  br i1 %tmp4, label %bb2, label %bb5
+
+bb5:                                              ; preds = %bb2
+  ret void
+}