Skip to content

[InstCombine] Merge foldFreezeIntoRecurrence into pushFreezeToPreventPoisonFromPropagating#171435

Closed
c-rhodes wants to merge 5 commits into
llvm:mainfrom
c-rhodes:instcombine-merge-freeze-phi-handling
Closed

[InstCombine] Merge foldFreezeIntoRecurrence into pushFreezeToPreventPoisonFromPropagating#171435
c-rhodes wants to merge 5 commits into
llvm:mainfrom
c-rhodes:instcombine-merge-freeze-phi-handling

Conversation

@c-rhodes
Copy link
Copy Markdown
Contributor

@c-rhodes c-rhodes commented Dec 9, 2025

No description provided.

@c-rhodes
Copy link
Copy Markdown
Contributor Author

c-rhodes commented Dec 9, 2025

@zyw-bot csmith-fuzz

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 9, 2025

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:
git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef([^a-zA-Z0-9_-]|$)|UndefValue::get)' 'HEAD~1' HEAD llvm/lib/Transforms/InstCombine/InstructionCombining.cpp llvm/test/Transforms/InstCombine/freeze.ll

The following files introduce new uses of undef:

  • llvm/test/Transforms/InstCombine/freeze.ll

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

Looking at this code again the Use/User distinction is a bit confusing,
but I'm pretty sure if we bail when CanPushFreeze returns false for the
PHI it should bring the behaviour inline with what was happening when
this code was part of foldFreezeIntoRecurrence.

Change was causing compile-time issues:
dtcxzyw/llvm-opt-benchmark#3129

The smallest being:
https://github.com/dtcxzyw/llvm-opt-benchmark/blob/main/bench/ffmpeg/original/avc.ll

which I reduced to: https://godbolt.org/z/6Gx58GfK1
@c-rhodes
Copy link
Copy Markdown
Contributor Author

in the first iteration there were some hangs dtcxzyw/llvm-opt-benchmark#3129, I got a reduced testcase for acv and fixed it with the latest commit.

Re-ran yesterday and there's more hangs: dtcxzyw/llvm-opt-benchmark#3130. I reduced miniz to: https://godbolt.org/z/cTqcxP9j4

it gets stuck in a loop of:

IC: Visiting:   %8 = zext i32 %7 to i64
IC: Mod =   %8 = zext i32 %7 to i64
    New =   %8 = zext nneg i32 %7 to i64
ADD:   %9 = freeze i64 %8
ADD:   %8 = zext nneg i32 %7 to i64
IC: Visiting:   %8 = zext nneg i32 %7 to i64
IC: Visiting:   %9 = freeze i64 %8
ADD DEFERRED:   %8 = zext i32 %7 to i64
ADD DEFERRED:   %7 = lshr i32 %6, 3
ADD DEFERRED:   %6 = add i32 %.2826, -8
ADD:   %8 = zext i32 %7 to i64

which is caused by

if (!Zext.hasNonNeg()) {
// If this zero extend is only used by a shift, add nneg flag.
if (Zext.hasOneUse() &&
SrcTy->getScalarSizeInBits() >
Log2_64_Ceil(DestTy->getScalarSizeInBits()) &&
match(Zext.user_back(), m_Shift(m_Value(), m_Specific(&Zext)))) {
Zext.setNonNeg();
return &Zext;
}
if (isKnownNonNegative(Src, SQ.getWithInstruction(&Zext))) {
Zext.setNonNeg();
return &Zext;
}
}

adding nneg then pushFreezeToPreventPoisonFromPropagating dropping it.

@nikic
Copy link
Copy Markdown
Contributor

nikic commented Dec 17, 2025

@c-rhodes I think it would great to commit tests for the infinite loop cases you reduced. That would help if this work gets picked up again in the future.

@c-rhodes
Copy link
Copy Markdown
Contributor Author

@c-rhodes I think it would great to commit tests for the infinite loop cases you reduced. That would help if this work gets picked up again in the future.

good idea, will do

c-rhodes added a commit to c-rhodes/llvm-project that referenced this pull request Dec 18, 2025
Adds a number of test cases where we have to be careful when pushing
freeze around. These first two tests are taken from llvm#157678 which failed
to land to due compile-time issues. The last two tests were reduced from
llvm-opt-benchmark workloads on llvm#171435 which was an attempt at
addressing underlying cause of the hangs.
@c-rhodes
Copy link
Copy Markdown
Contributor Author

@c-rhodes I think it would great to commit tests for the infinite loop cases you reduced. That would help if this work gets picked up again in the future.

good idea, will do

#172842

I've also added the test cases from #157678 you suggested.

c-rhodes added a commit that referenced this pull request Dec 19, 2025
Adds a number of test cases where we have to be careful when pushing
freeze around. These first two tests are taken from #157678 which failed
to land to due compile-time issues. The last two tests were reduced from
llvm-opt-benchmark workloads on #171435 which was an attempt at
addressing underlying cause of the hangs.
mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Dec 19, 2025
Adds a number of test cases where we have to be careful when pushing
freeze around. These first two tests are taken from llvm#157678 which failed
to land to due compile-time issues. The last two tests were reduced from
llvm-opt-benchmark workloads on llvm#171435 which was an attempt at
addressing underlying cause of the hangs.
mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Jan 6, 2026
Adds a number of test cases where we have to be careful when pushing
freeze around. These first two tests are taken from llvm#157678 which failed
to land to due compile-time issues. The last two tests were reduced from
llvm-opt-benchmark workloads on llvm#171435 which was an attempt at
addressing underlying cause of the hangs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants