This is an archive of the discontinued LLVM Phabricator instance.

[VP] vp intrinsics are not speculatable
ClosedPublic

Authored by simoll on May 10 2022, 2:40 AM.

Details

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.

Diff Detail

Event Timeline

simoll created this revision.May 10 2022, 2:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 2:40 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
simoll published this revision for review.May 10 2022, 2:42 AM
simoll added projects: Restricted Project, Restricted Project.
simoll updated this revision to Diff 428321.May 10 2022, 2:46 AM

Re-enable crossed out RUN: lines

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 updated this revision to Diff 430923.May 20 2022, 2:34 AM
simoll marked 3 inline comments as done.
  • 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)?

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

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 updated this revision to Diff 431976.May 25 2022, 7:10 AM
simoll edited the summary of this revision. (Show Details)

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.

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.

Ping. Can we land this patch?

frasercrmck accepted this revision.May 30 2022, 2:41 AM

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.

This revision is now accepted and ready to land.May 30 2022, 2:41 AM
This revision was landed with ongoing or failed builds.May 30 2022, 3:20 AM
This revision was automatically updated to reflect the committed changes.

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").