[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