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

3–4

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

3–4

+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

     return ConstantExpr::getNot(C);
 
   // Second: If the condition is already inverted, return the original value
-  if (match(Condition, m_Not(m_Value(Condition))))
-    return Condition;
+  Value *NotCondition;
+  if (match(Condition, m_Not(m_Value(NotCondition))))
+    return NotCondition;
 
   if (Instruction *Inst = dyn_cast<Instruction>(Condition)) {
     // Third: Check all the users for an invert

test/Transforms/StructurizeCFG/invert-condition.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -structurizecfg %s | FileCheck %s
+
+define void @invert_condition(i1 %arg) {
+; CHECK-LABEL: @invert_condition(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[TMP:%.*]] = load volatile float, float addrspace(1)* undef
+; CHECK-NEXT:    [[TMP1:%.*]] = load volatile float, float addrspace(1)* undef
+; CHECK-NEXT:    br label [[BB2:%.*]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[TMP3:%.*]] = fcmp oge float [[TMP]], [[TMP1]]
+; CHECK-NEXT:    [[TMP4:%.*]] = xor i1 [[ARG:%.*]], [[TMP3]]
+; CHECK-NEXT:    [[TMP0:%.*]] = xor i1 [[TMP4]], true
+; CHECK-NEXT:    br i1 [[TMP0]], label [[BB5:%.*]], label [[BB2]]
+; CHECK:       bb5:
+; CHECK-NEXT:    ret void
+;
+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
+}