Skip to content

[CanonicalizeFreezeInLoops] fix duplicate removal #74716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 44 additions & 16 deletions llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,10 @@
//===----------------------------------------------------------------------===//

#include "llvm/Transforms/Utils/CanonicalizeFreezeInLoops.h"
#include "llvm/ADT/DenseMapInfo.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Analysis/IVDescriptors.h"
#include "llvm/Analysis/LoopAnalysisManager.h"
#include "llvm/Analysis/LoopInfo.h"
Expand Down Expand Up @@ -66,19 +67,6 @@ class CanonicalizeFreezeInLoopsImpl {
ScalarEvolution &SE;
DominatorTree &DT;

struct FrozenIndPHIInfo {
// A freeze instruction that uses an induction phi
FreezeInst *FI = nullptr;
// The induction phi, step instruction, the operand idx of StepInst which is
// a step value
PHINode *PHI;
BinaryOperator *StepInst;
unsigned StepValIdx = 0;

FrozenIndPHIInfo(PHINode *PHI, BinaryOperator *StepInst)
: PHI(PHI), StepInst(StepInst) {}
};

// Can freeze instruction be pushed into operands of I?
// In order to do this, I should not create a poison after I's flags are
// stripped.
Expand All @@ -99,6 +87,46 @@ class CanonicalizeFreezeInLoopsImpl {

} // anonymous namespace

namespace llvm {

struct FrozenIndPHIInfo {
// A freeze instruction that uses an induction phi
FreezeInst *FI = nullptr;
// The induction phi, step instruction, the operand idx of StepInst which is
// a step value
PHINode *PHI;
BinaryOperator *StepInst;
unsigned StepValIdx = 0;

FrozenIndPHIInfo(PHINode *PHI, BinaryOperator *StepInst)
: PHI(PHI), StepInst(StepInst) {}

bool operator==(const FrozenIndPHIInfo &Other) { return FI == Other.FI; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function looks unused / unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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; }
           ~~~~~~^~~~~~~~~~~

};

template <> struct DenseMapInfo<FrozenIndPHIInfo> {
static inline FrozenIndPHIInfo getEmptyKey() {
return FrozenIndPHIInfo(DenseMapInfo<PHINode *>::getEmptyKey(),
DenseMapInfo<BinaryOperator *>::getEmptyKey());
}

static inline FrozenIndPHIInfo getTombstoneKey() {
return FrozenIndPHIInfo(DenseMapInfo<PHINode *>::getTombstoneKey(),
DenseMapInfo<BinaryOperator *>::getTombstoneKey());
}

static unsigned getHashValue(const FrozenIndPHIInfo &Val) {
return DenseMapInfo<FreezeInst *>::getHashValue(Val.FI);
};

static bool isEqual(const FrozenIndPHIInfo &LHS,
const FrozenIndPHIInfo &RHS) {
return LHS.FI == RHS.FI;
};
};

} // end namespace llvm

// Given U = (value, user), replace value with freeze(value), and let
// SCEV forget user. The inserted freeze is placed in the preheader.
void CanonicalizeFreezeInLoopsImpl::InsertFreezeAndForgetFromSCEV(Use &U) {
Expand Down Expand Up @@ -126,7 +154,7 @@ bool CanonicalizeFreezeInLoopsImpl::run() {
if (!L->isLoopSimplifyForm())
return false;

SmallVector<FrozenIndPHIInfo, 4> Candidates;
SmallSetVector<FrozenIndPHIInfo, 4> Candidates;

for (auto &PHI : L->getHeader()->phis()) {
InductionDescriptor ID;
Expand Down Expand Up @@ -155,7 +183,7 @@ bool CanonicalizeFreezeInLoopsImpl::run() {
if (auto *FI = dyn_cast<FreezeInst>(U)) {
LLVM_DEBUG(dbgs() << "canonfr: found: " << *FI << "\n");
Info.FI = FI;
Candidates.push_back(Info);
Candidates.insert(Info);
}
};
for_each(PHI.users(), Visit);
Expand Down
32 changes: 32 additions & 0 deletions llvm/test/Transforms/CanonicalizeFreezeInLoops/duplicate_remove.ll
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
; RUN: opt < %s --passes=canon-freeze -S | FileCheck %s

define void @check_duplicate_removal(i32 %n) {
; CHECK-LABEL: define void @check_duplicate_removal(
; CHECK-SAME: i32 [[N:%.*]]) {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[N_FROZEN:%.*]] = freeze i32 [[N]]
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
; CHECK-NEXT: [[T1:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[T3:%.*]], [[LOOP]] ]
; CHECK-NEXT: [[T2:%.*]] = phi i32 [ 0, [[ENTRY]] ], [ [[T3]], [[LOOP]] ]
; CHECK-NEXT: [[T3]] = add i32 [[N_FROZEN]], [[T2]]
; CHECK-NEXT: [[COND:%.*]] = icmp eq i32 [[T2]], 0
; CHECK-NEXT: br i1 [[COND]], label [[LOOP]], label [[EXIT:%.*]]
; CHECK: exit:
; CHECK-NEXT: ret void
;
entry:
br label %loop

loop:
%t1 = phi i32 [ 0, %entry], [%t3, %loop ]
%t2 = phi i32 [ 0, %entry], [%t3, %loop ]
%t3 = add i32 %n, %t2
%.fr = freeze i32 %t3
%cond = icmp eq i32 %t2, 0
br i1 %cond, label %loop, label %exit

exit:
ret void
}