[VPlan] Strip bad TODO in VPTransformState::get (NFC)

[artagnon]

It's impossible to create a VPInstruction::Broadcast at this point, since we need to return an IR Value, which would only be possible after the VPInstruction is executed.

[llvmbot]

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Ramkumar Ramachandra (artagnon)

Changes

It's impossible to create a VPInstruction::Broadcast at this point, since we need to return an IR Value, which would only be possible after the VPInstruction is executed.

---

Full diff: https://github.com/llvm/llvm-project/pull/166146.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/VPlan.cpp (-1)

diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 428a8f4c1348f..84c9c9c2de645 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -319,7 +319,6 @@ Value *VPTransformState::get(const VPValue *Def, bool NeedsScalar) {
 
   // We need to construct the vector value for a single-scalar value by
   // broadcasting the scalar to all lanes.
-  // TODO: Replace by introducing Broadcast VPInstructions.
   assert(IsSingleScalar && "must be a single-scalar at this point");
   // Set the insert point after the last scalarized instruction or after the
   // last PHI, if LastInst is a PHI. This ensures the insertelement sequence

[lukel97]

I presume the point of that TODO is not that it should emit a VPInstruction::Broadcast at that point, but all single scalar values should be wrapped in a VPInstruction::Broadcast before execution. So VPTransformState::get should never have to emit a splat.

[artagnon]

Yes, that's a good point.

[fhahn]

I presume the point of that TODO is not that it should emit a VPInstruction::Broadcast at that point, but all single scalar values should be wrapped in a VPInstruction::Broadcast before execution. So VPTransformState::get should never have to emit a splat.

Yep, that's the intention of the TODO

[artagnon]

I presume the point of that TODO is not that it should emit a VPInstruction::Broadcast at that point, but all single scalar values should be wrapped in a VPInstruction::Broadcast before execution. So VPTransformState::get should never have to emit a splat.

Yep, that's the intention of the TODO

Should we create a new VPTransform to wrap single-scalars in broadcasts?

[lukel97]

I presume the point of that TODO is not that it should emit a VPInstruction::Broadcast at that point, but all single scalar values should be wrapped in a VPInstruction::Broadcast before execution. So VPTransformState::get should never have to emit a splat.

Yep, that's the intention of the TODO

Should we create a new VPTransform to wrap single-scalars in broadcasts?

I think materializeBroadcasts does part of this? I'm not super familiar with it though, maybe it doesn't handle all single scalars yet.

[artagnon]

I presume the point of that TODO is not that it should emit a VPInstruction::Broadcast at that point, but all single scalar values should be wrapped in a VPInstruction::Broadcast before execution. So VPTransformState::get should never have to emit a splat.

Yep, that's the intention of the TODO

Should we create a new VPTransform to wrap single-scalars in broadcasts?

I think materializeBroadcasts does part of this? I'm not super familiar with it though, maybe it doesn't handle all single scalars yet.

Hm, strange. I wonder why it only does it for live-ins and for the Plan's entry, and not other recipes created in VPlan.