[SLP] Make getSameOpcode support interchangeable instructions. #133888
Conversation
…132887) We use the term "interchangeable instructions" to refer to different operators that have the same meaning (e.g., `add x, 0` is equivalent to `mul x, 1`). Non-constant values are not supported, as they may incur high costs with little benefit. --------- Co-authored-by: Alexey Bataev <a.bataev@gmx.com>
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/test/Transforms/SLPVectorizer/X86/BinOpSameOpcodeHelper.ll llvm/test/Transforms/SLPVectorizer/isOpcodeOrAlt.ll llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp llvm/test/Transforms/SLPVectorizer/AArch64/vec3-base.ll llvm/test/Transforms/SLPVectorizer/RISCV/reversed-strided-node-with-external-ptr.ll llvm/test/Transforms/SLPVectorizer/RISCV/vec3-base.ll llvm/test/Transforms/SLPVectorizer/X86/barriercall.ll llvm/test/Transforms/SLPVectorizer/X86/bottom-to-top-reorder.ll llvm/test/Transforms/SLPVectorizer/X86/buildvector-postpone-for-dependency.ll llvm/test/Transforms/SLPVectorizer/X86/bv-shuffle-mask.ll llvm/test/Transforms/SLPVectorizer/X86/extract-scalar-from-undef.ll llvm/test/Transforms/SLPVectorizer/X86/extractcost.ll llvm/test/Transforms/SLPVectorizer/X86/gathered-delayed-nodes-with-reused-user.ll llvm/test/Transforms/SLPVectorizer/X86/minbitwidth-drop-wrapping-flags.ll llvm/test/Transforms/SLPVectorizer/X86/multi-extracts-bv-combined.ll llvm/test/Transforms/SLPVectorizer/X86/non-scheduled-inst-reused-as-last-inst.ll llvm/test/Transforms/SLPVectorizer/X86/propagate_ir_flags.ll llvm/test/Transforms/SLPVectorizer/X86/reduced-val-vectorized-in-transform.ll llvm/test/Transforms/SLPVectorizer/X86/reorder_diamond_match.ll llvm/test/Transforms/SLPVectorizer/X86/shuffle-mask-emission.ll llvm/test/Transforms/SLPVectorizer/X86/vec3-base.ll llvm/test/Transforms/SLPVectorizer/X86/vect_copyable_in_binops.ll llvm/test/Transforms/SLPVectorizer/alternate-opcode-sindle-bv.ll llvm/test/Transforms/SLPVectorizer/resized-alt-shuffle-after-minbw.ll llvm/test/Transforms/SLPVectorizer/shuffle-mask-resized.llThe following files introduce new uses of undef:
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 In tests, avoid using 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. |
|
I test the following libraries. |
|
ping |
| static bool allSameOpcode(ArrayRef<Value *> VL) { | ||
| auto *It = find_if(VL, IsaPred<Instruction>); | ||
| if (It == VL.end()) | ||
| return true; | ||
| Instruction *MainOp = cast<Instruction>(*It); | ||
| unsigned Opcode = MainOp->getOpcode(); | ||
| bool IsCmpOp = isa<CmpInst>(MainOp); | ||
| CmpInst::Predicate BasePred = IsCmpOp ? cast<CmpInst>(MainOp)->getPredicate() | ||
| : CmpInst::BAD_ICMP_PREDICATE; | ||
| return std::all_of(It, VL.end(), [&](Value *V) { | ||
| if (auto *CI = dyn_cast<CmpInst>(V)) | ||
| return BasePred == CI->getPredicate(); | ||
| if (auto *I = dyn_cast<Instruction>(V)) | ||
| return I->getOpcode() == Opcode; | ||
| return isa<PoisonValue>(V); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
We have a function isAlternateInstruction, can we use it here?
There was a problem hiding this comment.
Cannot.
isAlternateInstruction consider interchangeable instruction.
allSameOpcode does not consider interchangeable instruction.
There was a problem hiding this comment.
Can you add a parameter to isAlternateInstruction to skip interchangeable instructions to avoid code duplication?
There was a problem hiding this comment.
Cannot implement allSameOpcode by isAlternateInstruction.
The MainOp and AltOp of isAlternateInstruction come from getSameOpcode, but getSameOpcode already takes interchangeable instructions into account.
However, the current allSameOpcode "refinds" MainOp and AltOp without considering alternate or interchangeable instructions.
|
@HanKuanChen and @alexey-bataev : Result: |
…133888) We use the term "interchangeable instructions" to refer to different operators that have the same meaning (e.g., `add x, 0` is equivalent to `mul x, 1`). Non-constant values are not supported, as they may incur high costs with little benefit. --------- Co-authored-by: Alexey Bataev <a.bataev@gmx.com>
llvm#133888)" This reverts commit 123993f.
|
Bisected a libc++ strict weak ordering check failure to this commit on the following IR: it's pointing at the |
|
OK, but can you confirm that the issue has been fixed? If you aren't confident that both issues have been fixed, it's our project policy to revert first and ask questions later: At EuroLLVM I spoke to multiple other individuals for whom the SLP vectorizer is the #1 source of toolchain instability. I have seen issues reported to SLP v developers with responses like along the lines of, this feedback is too late, we've already moved on, it will be hard to rollback. IMO these are not valid excuses for allowing tip-of-tree to be unstable. When a user reports a correctness issue in LLVM, as compiler developers, correctness is paramount, and we should take it seriously. So, with that in mind, I would appreciate confirmation of whether the reported issue has been fixed. |
|
I built using commit ea0dbee, and no bugs were found. |
|
I can still reproduce this at main (as of a couple minutes ago). You need |
Hope f427890 will fix it |
|
If possible, let's try to keep a new optimization disabled by default and let it soak for a bit (maybe a week or two?) during which more testing can be done. |
|
thanks for the fix @alexey-bataev, confirmed that fixes the repro |
@rnk: Thank you for raising this! This is the case for us. We have an out of tree backend and we merge from upstream trunk llvm several times per day, and fairly often we see new problems with the SLP vectorizer. When I report a problem it does get fixed quickly though so thumbs up for that, but that the issues appear in the first place is turning into a problem. Digging out a reproducer, and then converting it to something that reproduces on an in-tree target often takes an effort since the pass depends quite a bit on the target. We have even raised the question internally if we should disable SLP vectorizer since the maintenance cost of dealing with the crashes is sticking out. Of course bugs and problems will always occur but SLP vectorizer unfortunately stands out in a negative way for us due to the asserts we sometimes see after merging new patches. |
I'm sorry this causes so many troubles on your end, and I appreciate your help with fixing the bugs. I hope this situation will be improved soon. There is just one big functional change expected, all other changes will be mostly related to redesign and fixes. |
We use the term "interchangeable instructions" to refer to different
operators that have the same meaning (e.g.,
add x, 0is equivalent tomul x, 1).Non-constant values are not supported, as they may incur high costs with
little benefit.
Co-authored-by: Alexey Bataev a.bataev@gmx.com