[CanonicalizeFreezeInLoops] fix duplicate removal

[Friedrich20]

This PR fixes #74572 where the freeze instruction could be found twice by the pass CanonicalizeFreezeInLoops, and then the compiling may crash in second removal since the instruction has already gone.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Wei Tao (Friedrich20)

Changes

This PR fixes #74572 where the the freeze instruction could be found twice by the pass CanonicalizeFreezeInLoops, and then the compiling may crash in second removal since the instruction has already gone.

---

Full diff: https://github.com/llvm/llvm-project/pull/74716.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp (+10-3)

diff --git a/llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp b/llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp
index fb4d828853772..ff1eb17e0c248 100644
--- a/llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp
+++ b/llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp
@@ -151,11 +151,18 @@ bool CanonicalizeFreezeInLoopsImpl::run() {
       }
     }
 
+    bool Exist = false;
     auto Visit = [&](User *U) {
       if (auto *FI = dyn_cast<FreezeInst>(U)) {
-        LLVM_DEBUG(dbgs() << "canonfr: found: " << *FI << "\n");
-        Info.FI = FI;
-        Candidates.push_back(Info);
+        for (const auto &Candidate : Candidates) {
+          auto *FI_cand = Candidate.FI;
+          Exist = (FI_cand == FI) ? true : Exist;
+        }
+        if (!Exist) {
+          LLVM_DEBUG(dbgs() << "canonfr: found: " << *FI << "\n");
+          Info.FI = FI;
+          Candidates.push_back(Info);
+        }
       }
     };
     for_each(PHI.users(), Visit);

[nikic]

Can you please add a (reduced) test case in llvm/test/Transforms/CanonicalizeFreezeInLoops/?

[fhahn]

How about instead changing Candidates to a SetVector as suggested by @nikic at the issue? I suppose it would require specializing DenseMapInfo for FrozenIndPHIInfo. See https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Analysis/MemoryLocation.h#L330 for an example

[Friedrich20]

@fhahn ,
The fix has been refactored by using SmallSetVector.
Thanks for the example that really helps a lot!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[nikic]

This function looks unused / unnecessary?

[Friedrich20]

@nikic ,
I have tested this fix on both llvmorg-15.0.7 and the latest main branch. Just as you said, this function is totally unused on llvm15. However, it is required by the main branch to overload this function to support operator == between type and const type.

/home/taowei/repo/llvm-project/llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:184:31:   required from here
/usr/include/c++/7/bits/predefined_ops.h:241:17: error: no match for ‘operator==’ (operand types are ‘llvm::FrozenIndPHIInfo’ and ‘const llvm::FrozenIndPHIInfo’)
  { return *__it == _M_value; }
           ~~~~~~^~~~~~~~~~~

[Friedrich20]

@nikic,
many thanks!