[llvm] [SLP] NFC. Replace TreeEntry::setOperandsInOrder with VLOperands. (PR #113880)
Han-Kuan Chen via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 29 01:22:28 PST 2024
HanKuanChen wrote:
> > Why do we need to check the ID while the old code does not have to?
>
> Because the load code explicitly worked only for CallInst
But we have a `if (auto *CI = dyn_cast<CallInst>(VL[0])) {` check here.
> > Because only the commutative intrinsics check it in the old code.
>
> The non-commutative intrinsics also need to have a check for isVectorIntrinsicWithScalarOpAtArg
If we add `isVectorIntrinsicWithScalarOpAtArg` check for non-commutative intrinsics, `llvm/test/Transforms/SLPVectorizer/X86/extract_in_tree_user.ll` will fail even without this PR. The bug is not caused by this PR.
> > I think we should NOT add isVectorIntrinsicWithScalarOpAtArg check for setOperand
>
> The check should be there or this whole patch should be dropped. We should not create pseudo tree node for the scalar instructions
If LLVM has an intrinsic in the future and
- the intrinsic is commutative
- one of the operand is `isVectorIntrinsicWithScalarOpAtArg`, and the operand is NOT `Constant`
SLP will get fail because the operand is NOT in `TreeEntry` and it will trigger the assert in
```
#if !defined(NDEBUG) || defined(EXPENSIVE_CHECKS)
// Check that all schedulable entities got scheduled
for (auto *I = BS->ScheduleStart; I != BS->ScheduleEnd; I = I->getNextNode()) {
ScheduleData *SD = BS->getScheduleData(I);
if (SD && SD->isSchedulingEntity() && SD->hasValidDependencies())
assert(SD->IsScheduled && "must be scheduled at this point");
}
#endif
```
Have https://github.com/llvm/llvm-project/commit/664f2d333ee051c30693b95ff8c59cf262c35cce will cause a potential bug in the future.
https://github.com/llvm/llvm-project/pull/113880
More information about the llvm-commits
mailing list