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

[Simon Moll via Phabricator via llvm-commits]

simoll added inline comments.


================
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,
----------------
frasercrmck wrote:
> 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.
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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125296