[PATCH] D125296: [VP] vp intrinsics are not speculatable

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

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.



================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:467
+  /// Return the same result as isSafeToSpeculativelyExecute under the
+  /// assumption that the opcode of the instruction is Opcode.
+  bool isSafeToSpeculativelyExecuteWithOpcode(unsigned Opcode,
----------------
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.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:469
+  bool isSafeToSpeculativelyExecuteWithOpcode(unsigned Opcode,
+                                    const Operator *Inst,
+                                    const Instruction *CtxI = nullptr,
----------------
Need to adjust formatting here


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1514
 // Floating-point arithmetic.
 let IntrProperties =
+    [IntrNoMem, IntrNoSync, IntrWillReturn] in {
----------------
Join lines?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125296/new/

https://reviews.llvm.org/D125296