Skip to content

Conversation

RiverDave
Copy link
Collaborator

Only one thing to note related to the shuffles performed for these intrinsics. We could tackle this in a different PR tho.

OG's builder creates unary shuffles by passing a poison value for the second vector operand.

/// Create a unary shuffle. The second vector operand of the IR instruction
/// is poison.
Value *CreateShuffleVector(Value *V, ArrayRef<int> Mask,
                             const Twine &Name = “”) {
    return CreateShuffleVector(V, PoisonValue::get(V->getType()), Mask, Name);
}

In CIR, we currently work around this by duplicating the single vector argument as both operands:

    cir::VecShuffleOp createVecShuffle(mlir::Location loc, mlir::Value vec1,
                                     llvm::ArrayRef<int64_t> mask) {
    // FIXME(cir): Support use cir.vec.shuffle with single vec
    // Workaround: pass Vec as both vec1 and vec2
    return createVecShuffle(loc, vec1, vec1, mask);
}

While this is just a minor nit and I don’t believe we provoke a semantic difference, should we revisit the workaround stated in the comment for parity with OG?

@bcardosolopes
Copy link
Member

While this is just a minor nit and I don’t believe we provoke a semantic difference, should we revisit the workaround stated in the comment for parity with OG?

That'd be great for a new PR!

@bcardosolopes
Copy link
Member

Good after macOS-13 tests pass (also conflict resolution needed)

@bcardosolopes
Copy link
Member

tests still failing (only the windows one is ignorable at this point)

@RiverDave
Copy link
Collaborator Author

RiverDave commented Aug 21, 2025

tests still failing (only the windows one is ignorable at this point)

Yeah, I'm still trying to figure this out. For some reason I cannot reproduce this failure locally. Will investigate this further

@bcardosolopes
Copy link
Member

Needs come conflict resolution but good to go!

@RiverDave
Copy link
Collaborator Author

Needs come conflict resolution but good to go!

Should be fixed now

@RiverDave
Copy link
Collaborator Author

RiverDave commented Aug 26, 2025

Hey @bcardosolopes, I’ve noticed a lot of merge conflicts popping up since my work on intrinsics is happening in parallel with other PRs. Would it make sense to grant me write access so I can help resolve conflicts more smoothly and keep things moving and merging? No worries if not, I’ll keep rebasing as usual. Just thought I’d check. (For context, I do already have upstream (llvm/llvm-project#131971) write since I contributed to clang-tidy. let me know if that helps.)

@bcardosolopes
Copy link
Member

Would it make sense to grant me write access so I can help resolve conflicts more smoothly and keep things moving and merging?

Do you mean for merging your PRs faster once approved? If so I'm happy to do it (just making sure we are talking about the same thing)

@RiverDave
Copy link
Collaborator Author

Do you mean for merging your PRs faster once approved?

Ofc, I meant for merging once approved :)

@bcardosolopes
Copy link
Member

Done!

@bcardosolopes bcardosolopes merged commit 90fc6c4 into llvm:main Aug 27, 2025
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants