Clean up code based on reviewer feedback.

Add special case handling:

Hang on a bit.
Let me see if I can find more issues in ConstantFoldBinaryInstruction.

Similar issue , current upstream crash at void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed.

Yes, this transformation will need to go in InstCombine.
We should probably implement this when constant scalable vector support is ready, so that the effect will be most obvious.

In D71389#1781620, @spatel wrote:

LGTM. I don't know how many of these patches are needed, but it would be slightly more efficient to create a "vscale.ll" test file and put all of the related tests in that 1 file.

Current upstream crash with llvm/lib/IR/Value.cpp:404: void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed.

Please let me know if you have any further concerns?

"The main problem is the fix should be applied to all duplication even if BB is not removed."
-> Make sense, thank you for pointing this out.

Thank you Sanjay!

Addressed review feedback.

current upstream crash with "void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed."

Hey @Carrot , really appreciated your time on it.

I would like to merge this patch, please let me know if there are any objections?

Updated test checking.

Any objections with this approach?

Add reduced test case

Addressed reviewer feedback -- add more explanation in comments.

Guys, any thoughts on this?

Just realize I forget to mention how the BB is removed.

Actually fillWorkLists is not called for the deleted BB, but the MBBs of BBChain. The BB is deleted.
Here BBChain will have an unreleased UnscheduledPredecessors counter, when calling fillWorkLists for any MBBs within BBChain, this assertion will happen.

Adding some more detailed explanation:

This problem caused assertion on: Assertion `BlockToChain[&MBB]->UnscheduledPredecessors == 0 && "expect unschedPred to be 0\n"' failed.

Not a good solution

This is just FYI.

Another note, for older generation X86 target, e.g., haswell, cmove indeed has latency 2. But able to achieve comparable uOps Per Cycle
same test input
clang clampNegToZero.ll -O2 -target x86_64 -march=haswell -S -o - | llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=haswell
before

llvm-mca performance result for general folding:

llvm-mca results for more general folding pattern

make folding more general

make test more general

resolved reviews feedback

resolved reviews feedback

move test "extra_use_sub" to positive case

Similar to D67799

For X86, AArch64 and ARM target, backend produce better ASM with this transformation. Please refer to below examples:

E.g., vmin generation for ARM target

E.g., vmax generation for ARM target

forget to update the test comment, sorry

test case foo1_and_signbit_lshr_without_shifting_signbit_not_pwr2() aim to check that we are not creating additional shift instruction when fold fails.

I think we should only create signbit shift instruction when fold is supposed to happen.

In D64275#1575312, @lebedev.ri wrote:

Thanks for working on this!
This is for sure missing some one-use checks (i.e. increases instruction count).
Could you please add 5 copies of @foo1_and_signbit_lshr_without_shifting_signbit, each one with different instruction having extra use?

Addressed review comments

Generalize InstCombiner::foldAndOrOfICmpsOfAndWithPow2() in D64275