[VP] vp intrinsics are not speculatable

[simoll]

Summary

VP intrinsics show UB if the %evl parameter is out of bounds - they must not carry the speculatable attribute. The out-of-bounds UB disappears when the %evl parameter is expanded into the mask or expansion replaces the entire VP intrinsic with non-VP code.

This patch

• Removes the speculatable attribute on all VP intrinsics.
• Generalizes the isSafeToSpeculativelyExecute function to let VP expansion know whether the VP intrinsic replacement will be speculatable. VP expansion may only discard %evl where this is the case.

[simoll]

Re-enable crossed out RUN: lines

[frasercrmck]

This is what I was thinking of. Short of speculatively materializing an instruction only to throw it away, I don't see a better way.

llvm/include/llvm/Analysis/ValueTracking.h

467

I think we need extra documentation about Opcode and Inst how they relate and the constraints placed on their relationship.

What if Opcode expects a certain operand and Inst doesn't have it? Or if the operand is of a different type? Basically, is Opcode the truth or is Inst? In practice we're currently using this when Inst has more redundant operands than Opcode expects but until that point there's a 1:1 match.

I'm just a bit wary of introducing something under-specified and prone to undefined/unreliable behaviour. It's probably really hard to enforce in code, but a comment might at least prevent egregious usage.

469

Need to adjust formatting here

llvm/include/llvm/IR/Intrinsics.td

1508–1509

Join lines?

[simoll]

• More documentation on isSafeToSpeculativelyExecuteWithOpcode
• VP Intrinsics cleanup in Intrinsics.td
• Rebased
• Formatting

llvm/include/llvm/Analysis/ValueTracking.h

467

I am a bit wary of the complexity of overriding the opcode, too. Hopefully, the additional comments and assertions help document this better. Maybe i should even mention the intended use case (overriding an intrinsic with an opcode)?

[simoll]

Note from syncup call: Split up into a fixup for vp reduction expansion and a second patch for the speculatable flag.

[simoll]

Please review the first of two sub patches: https://reviews.llvm.org/D126362

I will abandon this Diff once both sub patches have landed.

[simoll]

VP intrinsics show UB if the %evl parameter is out of bounds - they must not carry the speculatable attribute. The out-of-bounds UB disappears when the %evl parameter is expanded into the mask or expansion replaces the entire VP intrinsic with non-VP code.

This patch

• Removes the speculatable attribute on all VP intrinsics.
• Generalizes the isSafeToSpeculativelyExecute function to let VP expansion know whether the VP intrinsic replacement will be speculatable. VP expansion may only discard %evl where this is the case.

[simoll]

To clarify: This Diff is the second sub patch and ready for review. I changed my mind on the whole adding-another-sub-patch-and-abandoning-this-patch thing.

[simoll]

Ping. Can we land this patch?

[frasercrmck]

Hah I was actually writing LGTM when I got pulled away. I was gonna say this looks good to me - we can add more restrictions if/when we need them. I think what you've got now should set the foundations as best we can.

[mehdi_amini]

I reverted because it broke a bot.

The failure was shown on the pre-merge testing here by the way ("x64 debian > MLIR.Target/LLVMIR::llvmir-intrinsics.mlir").