Fix the new pattern according to the review

diffkemp/cli.py

@@ -153,7 +153,7 @@ def make_argument_parser():
                         "control-flow-only",
                         "inverse-conditions",
                         "reordered-bin-ops",
-                        "struct-changes"]
+                        "group-vars"]
 
     # Semantic patterns options.
     compare_ap.add_argument("--enable-pattern",

diffkemp/config.py

@@ -23,7 +23,7 @@ def __init__(
         control_flow_only=False,
         inverse_conditions=True,
         reordered_bin_ops=True,
-        struct_changes=True,
+        group_vars=True,
     ):
         """
         Create a configuration of built-in patterns.
@@ -42,7 +42,7 @@ def __init__(
         :param control_flow_only: Consider control-flow changes only.
         :param inverse_conditions: Inverted branch conditions.
         :param reordered_bin_ops: Match reordered binary operations.
-        :param struct_changes: Changes in local structs and variables.
+        :param group_vars: Grouping of local variables.
         """
         self.settings = {
             "struct-alignment": struct_alignment,
@@ -56,7 +56,7 @@ def __init__(
             "control-flow-only": control_flow_only,
             "inverse-conditions": inverse_conditions,
             "reordered-bin-ops": reordered_bin_ops,
-            "struct-changes": struct_changes,
+            "group-vars": group_vars,
         }
         self.resolve_dependencies()
 
@@ -104,7 +104,7 @@ def as_ffi_struct(self):
         ffi_struct.ControlFlowOnly = self.settings["control-flow-only"]
         ffi_struct.InverseConditions = self.settings["inverse-conditions"]
         ffi_struct.ReorderedBinOps = self.settings["reordered-bin-ops"]
-        ffi_struct.StructChanges = self.settings["struct-changes"]
+        ffi_struct.GroupVars = self.settings["group-vars"]
         return ffi_struct
 
 

diffkemp/simpll/Config.h

@@ -38,7 +38,7 @@ struct BuiltinPatterns {
     bool ControlFlowOnly = false;
     bool InverseConditions = true;
     bool ReorderedBinOps = true;
-    bool StructChanges = true;
+    bool GroupVars = true;
 };
 
 /// Tool configuration parsed from CLI options.

diffkemp/simpll/DifferentialFunctionComparator.cpp

@@ -506,28 +506,41 @@ int DifferentialFunctionComparator::cmpAllocs(const CallInst *CL,
 
 /// Check if the given instruction can be ignored (it does not affect
 /// semantics). Ignorable instructions are stored inside the replaced
-/// instructions map or the ignored instructions set, depending on whether
-/// they need a replacement.
+/// instructions map or the ignored instructions map, depending on whether
+/// they have a replacement.
 bool DifferentialFunctionComparator::maySkipInstruction(
         const Instruction *Inst) const {
-    if (Inst->isSafeToRemove()
-        && (Inst->getNumUses() == 0
-            || (config.Patterns.StructChanges
-                && isOnlyUsedByIgnorableMemInstructions(Inst)))) {
-        // We can skip instructions that are unused and safe to remove (i.e.
-        // not calls, stores, or terminators). If an instruction is used only
-        // as a pointer operand of memory instructions that may be ignored in
-        // the future, it may be skipped too.
-        ignoredInstructions.insert(Inst);
-        return true;
-    }
     if (isa<AllocaInst>(Inst)) {
-        ignoredInstructions.insert(Inst);
+        // An alloca can always be ignored without a replacement.
+        // It can later be "un-ignored" if it matches with an equal alloca
+        // when compared as an operand of another instruction.
+        ignoredInstructions.insert({Inst, true});
         return true;
     }
-    if (config.Patterns.ReorderedBinOps && isReorderableBinaryOp(Inst)
-        && maySkipReorderableBinaryOp(Inst)) {
-        ignoredInstructions.insert(Inst);
+    // Next, check if we can skip this instruction because all its users
+    // satisfy certain conditions. We define a list of conditions applicable
+    // for the instruction and then check that each user satisfies at least one.
+    std::vector<std::function<bool(const User *)>> Conditions;
+    if (config.Patterns.GroupVars && Inst->isSafeToRemove()) {
+        // If this is a safe instruction (not a store, call, or a terminator),
+        // we do not care about users that are extra memory instructions, i.e.,
+        // stores to or loads from ignored local variables. This is useful for
+        // skipping GEPs that return pointers to such variables.
+        Conditions.emplace_back(
+                [this](const User *User) { return isExtraMemInst(User); });
+    }
+    if (config.Patterns.ReorderedBinOps && isReorderableBinaryOp(Inst)) {
+        // If this is a reorderable binary operator, we do not care about users
+        // that are binary operators of the same kind. The full set of operands
+        // will be compared later, when the users are compared.
+        Conditions.emplace_back([Inst](const User *User) {
+            return hasOpcode(User, Inst->getOpcode());
+        });
+    }
+    if (!Conditions.empty() && usersSatisfyCondition(Inst, Conditions)) {
+        // If all users satisfy at least one of the conditions, we may
+        // ignore this instruction.
+        ignoredInstructions.insert({Inst, false});
         return true;
     }
     if (isCast(Inst)) {
@@ -542,10 +555,16 @@ bool DifferentialFunctionComparator::maySkipInstruction(
         return true;
     }
     if (auto Store = dyn_cast<StoreInst>(Inst)) {
-        if (config.Patterns.StructChanges
-            && hasIgnoredAllocaAsPtrOperand(Store)) {
-            // Skip a store if it points to a previously ignored alloca.
-            ignoredInstructions.insert(Store);
+        if (config.Patterns.GroupVars && hasIgnoredAllocaOp(Store)) {
+            // We can skip a store if it stores to an ignored local variable.
+            // This is allowed because any load from the same variable will
+            // also fail, and will have to be skipped and replaced with the
+            // stored value. As a result, the corresponding alloca cannot be
+            // "un-ignored" from this point onward, because that could result
+            // in not skipping a future load from the variable (its contents
+            // may differ from now).
+            ignoredInstructions.insert({Store, false});
+            ignoredInstructions[getAllocaOp(Store)] = false;
             return true;
         }
     }
@@ -676,7 +695,7 @@ bool DifferentialFunctionComparator::maySkipLoad(
                 Replacement = ReplacementCandidate;
                 checkPredecessors = false;
                 break;
-            } else if (mayStoreToMemoryAtPtr(&*it, Load->getPointerOperand())) {
+            } else if (mayStoreTo(&*it, Load->getPointerOperand())) {
                 // Fail if a possibly conflicting store or call is found.
                 return false;
             }
@@ -685,7 +704,9 @@ bool DifferentialFunctionComparator::maySkipLoad(
         if (checkPredecessors) {
             auto BBPredecessors = predecessors(BB);
             if (BBPredecessors.begin() == BBPredecessors.end()) {
-                // Fail if no more predecessors are available.
+                // Fail if no more predecessors are available. This means that
+                // we've reached the top of the function, and we haven't found
+                // a suitable replacement, so we cannot skip the load.
                 return false;
             }
             // Queue up all unvisited predecessors.
@@ -705,20 +726,6 @@ bool DifferentialFunctionComparator::maySkipLoad(
     return false;
 }
 
-/// Check whether the given reorderable binary operator can be skipped.
-/// It can only be skipped if all its users are binary operations
-/// of the same kind or ignorable stores.
-bool DifferentialFunctionComparator::maySkipReorderableBinaryOp(
-        const Instruction *Op) const {
-    return std::all_of(Op->user_begin(), Op->user_end(), [&](auto user) {
-        if (auto userBinOp = dyn_cast<BinaryOperator>(user))
-            return userBinOp->getOpcode() == Op->getOpcode();
-        if (auto userStore = dyn_cast<StoreInst>(user))
-            return hasIgnoredAllocaAsPtrOperand(userStore);
-        return false;
-    });
-}
-
 bool mayIgnoreMacro(std::string macro) {
     return ignoredMacroList.find(macro) != ignoredMacroList.end();
 }
@@ -1414,12 +1421,6 @@ int DifferentialFunctionComparator::cmpFieldAccess(
     if (!(GEPL && GEPR))
         RETURN_WITH_LOG(1);
 
-    // If we previously ignored allocation of memory that any of the two GEPs
-    // point to, fail (cmpValues would compare their pointer operands as equal).
-    if (hasIgnoredAllocaAsPtrOperand(GEPL)
-        || hasIgnoredAllocaAsPtrOperand(GEPR))
-        RETURN_WITH_LOG(1);
-
     if (!GEPL->hasAllConstantIndices() || !GEPR->hasAllConstantIndices())
         RETURN_WITH_LOG(1);
 
@@ -1472,10 +1473,36 @@ int DifferentialFunctionComparator::cmpValues(const Value *L,
     PREP_LOG("value", L, R);
 
     // Instructions that were ignored without replacement must never be compared
-    // as values. Two such instructions would always synchronize (even if not
-    // equal). Comparing with an already synchronized value would pollute the
-    // SN maps and complicate eventual undo.
-    if (ignoredInstructions.count(L) || ignoredInstructions.count(R)) {
+    // as values, since they were never synchronized in the first place.
+    // We can, however, try to compare them as operations, if it's permitted by
+    // the boolean value in the ignored instruction map.
+    auto IgnoredEnd = ignoredInstructions.end();
+    auto IgnoredItL = ignoredInstructions.find(L);
+    auto IgnoredItR = ignoredInstructions.find(R);
+    if (IgnoredItL != IgnoredEnd || IgnoredItR != IgnoredEnd) {
+        if (IgnoredItL != IgnoredEnd && IgnoredItR != IgnoredEnd
+            && IgnoredItL->second && IgnoredItR->second) {
+            // Both instructions are ignored and both can be "un-ignored".
+            // Try to compare them fully.
+            auto InstL = dyn_cast<Instruction>(IgnoredItL->first);
+            auto InstR = dyn_cast<Instruction>(IgnoredItR->first);
+            // The instructions must not be marked as ignored now, because
+            // comparison of operations calls cmpValues immediately again.
+            ignoredInstructions.erase(IgnoredItL);
+            ignoredInstructions.erase(IgnoredItR);
+            if (!cmpOperationsWithOperands(InstL, InstR)) {
+                RETURN_WITH_LOG_NEQ(0);
+            }
+            // If the comparison failed, restore the ignored instructions map
+            // and synchronization maps, which are polluted.
+            ignoredInstructions.insert({InstL, true});
+            ignoredInstructions.insert({InstR, true});
+            sn_mapL.erase(InstL);
+            sn_mapR.erase(InstR);
+            mappedValuesBySn.erase(sn_mapL.size());
+        }
+        // If only one of the instructions is ignored or they were compared
+        // as not equal, fail.
         RETURN_WITH_LOG_NEQ(1);
     }
 
@@ -2059,32 +2086,21 @@ bool DifferentialFunctionComparator::checkInverseCondUsers(
     return true;
 }
 
-/// Check whether the provided value is a previously ignored alloca.
-bool DifferentialFunctionComparator::isIgnoredAlloca(const Value *Val) const {
-    return isa<AllocaInst>(Val) && ignoredInstructions.count(Val);
-}
-
 /// Check whether the pointer operand of the given instruction (e.g., load)
 /// is a previously ignored alloca or a GEP pointing to one.
 template <typename InstType>
-bool DifferentialFunctionComparator::hasIgnoredAllocaAsPtrOperand(
+bool DifferentialFunctionComparator::hasIgnoredAllocaOp(
         const InstType *Inst) const {
-    const Value *Ptr = Inst->getPointerOperand();
-    if (auto GEP = dyn_cast<GetElementPtrInst>(Ptr))
-        return hasIgnoredAllocaAsPtrOperand(GEP);
-    return isIgnoredAlloca(Ptr);
+    if (auto Alloca = getAllocaOp(Inst))
+        return ignoredInstructions.count(Alloca);
+    return false;
 }
 
-/// Check whether all uses of the provided value are loads or stores which
-/// may be skipped.
-bool DifferentialFunctionComparator::isOnlyUsedByIgnorableMemInstructions(
-        const Value *Val) const {
-    return std::all_of(
-            Val->user_begin(), Val->user_end(), [&](const User *user) {
-                if (auto store = dyn_cast<StoreInst>(user))
-                    return hasIgnoredAllocaAsPtrOperand(store);
-                if (auto load = dyn_cast<LoadInst>(user))
-                    return hasIgnoredAllocaAsPtrOperand(load);
-                return false;
-            });
+/// Check whether the given instruction is an extra load or store.
+bool DifferentialFunctionComparator::isExtraMemInst(const User *Usr) const {
+    if (auto store = dyn_cast<StoreInst>(Usr))
+        return hasIgnoredAllocaOp(store);
+    if (auto load = dyn_cast<LoadInst>(Usr))
+        return hasIgnoredAllocaOp(load);
+    return false;
 }

diffkemp/simpll/DifferentialFunctionComparator.h

@@ -148,9 +148,15 @@ class DifferentialFunctionComparator : public FunctionComparator {
     mutable std::set<std::pair<const Value *, const Value *>> inverseConditions;
     mutable std::vector<std::pair<const PHINode *, const PHINode *>>
             phisToCompare;
+
+    /// Map of skipped instructions with replacements.
     mutable std::unordered_map<const Value *, const Value *>
             replacedInstructions;
-    mutable std::unordered_set<const Value *> ignoredInstructions;
+
+    /// Map of skipped instructions without replacements. The boolean value
+    /// specifies whether the ignored instruction can be later "un-ignored"
+    /// by being compared with an equal instruction as an operand.
+    mutable std::unordered_map<const Value *, bool> ignoredInstructions;
 
     /// Contains pairs of values mapped by synchronisation maps. Enables
     /// retrieval of mapped values based on assigned numbers.
@@ -217,8 +223,8 @@ class DifferentialFunctionComparator : public FunctionComparator {
 
     /// Check if the given instruction can be ignored (it does not affect
     /// semantics). Ignorable instructions are stored inside the replaced
-    /// instructions map or the ignored instructions set, depending on whether
-    /// they need a replacement.
+    /// instructions map or the ignored instructions map, depending on whether
+    /// they have a replacement.
     bool maySkipInstruction(const Instruction *Inst) const;
 
     /// Check whether the given cast can be ignored (it does not affect
@@ -230,14 +236,9 @@ class DifferentialFunctionComparator : public FunctionComparator {
     /// a repetitive variant of a previous load, or a load from an ignored
     /// local variable. Load replacements are stored inside the replaced
     /// instructions map.
-    bool maySkipLoad(const llvm::LoadInst *Load) const;
+    bool maySkipLoad(const LoadInst *Load) const;
 
-    /// Check whether the given reorderable binary operator can be skipped.
-    /// It can only be skipped if all its users are binary operations
-    /// of the same kind or ignorable stores.
-    bool maySkipReorderableBinaryOp(const Instruction *Op) const;
-
-    /// Retrieve the replacement for the given value from the replaced
+    /// Retrieve the replacement for the given instruction from the replaced
     /// instructions map. Try to generate the replacement if a bitcast is given.
     const Value *
             getReplacementValue(const Value *Replaced,
@@ -289,17 +290,13 @@ class DifferentialFunctionComparator : public FunctionComparator {
     /// false otherwise.
     bool checkInverseCondUsers(const Instruction *inst) const;
 
-    /// Check whether the provided value is a previously ignored alloca.
-    bool isIgnoredAlloca(const Value *Val) const;
-
     /// Check whether the pointer operand of the given instruction (e.g., load)
     /// is a previously ignored alloca or a GEP pointing to one.
     template <typename InstType>
-    bool hasIgnoredAllocaAsPtrOperand(const InstType *Inst) const;
+    bool hasIgnoredAllocaOp(const InstType *Inst) const;
 
-    /// Check whether all uses of the provided value are loads or stores which
-    /// may be skipped.
-    bool isOnlyUsedByIgnorableMemInstructions(const Value *Val) const;
+    /// Check whether the given instruction is an extra load or store.
+    bool isExtraMemInst(const User *Usr) const;
 };
 
 #endif // DIFFKEMP_SIMPLL_DIFFERENTIALFUNCTIONCOMPARATOR_H

diffkemp/simpll/SimpLL.cpp

@@ -115,9 +115,9 @@ cl::opt<bool> ReorderedBinOpsOpt(
         "reordered-bin-ops",
         cl::desc("Enable reordered binary operations pattern."),
         cl::cat(BuiltinPatternsCategory));
-cl::opt<bool> StructChangesOpt("struct-changes",
-                               cl::desc("Enable struct changes pattern."),
-                               cl::cat(BuiltinPatternsCategory));
+cl::opt<bool> GroupVarsOpt("group-vars",
+                           cl::desc("Enable variable grouping pattern."),
+                           cl::cat(BuiltinPatternsCategory));
 
 /// Add suffix to the file name.
 /// \param File Original file name.
@@ -166,7 +166,7 @@ int main(int argc, const char **argv) {
                              .ControlFlowOnly = ControlFlowOnlyOpt,
                              .InverseConditions = InverseConditionsOpt,
                              .ReorderedBinOps = ReorderedBinOpsOpt,
-                             .StructChanges = StructChangesOpt};
+                             .GroupVars = GroupVarsOpt};
 
     // Parse --fun option
     auto FunName = parseFunOption();