[PATCH] D59657: [LangRef] Clarify codegen expectations for intrinsics with fp/integer-only overloads

[Amara Emerson via Phabricator via llvm-commits]

aemerson marked an inline comment as done.
aemerson added a comment.

In D59657#1438384 <https://reviews.llvm.org/D59657#1438384>, @hfinkel wrote:

> I don't understand what this is supposed to mean to who the audience of this statement is?


Clearly the wording isn't good enough then, but here's the motivating discussion: http://lists.llvm.org/pipermail/llvm-dev/2019-March/130903.html

The tl;dr is that we agreed that intrinsics which need different codegen based *only* on whether a intrinsic operand type is integer or fp is not supported.

`call @llvm.foo(<4 x i32> %p)` and `call @llvm.foo(<4 x float> %p)` should not be supported by the code generator, as the only thing that distinguishes them is the fp/integer type. With llvm::Instruction we have separate opcodes for fp/int operations, e.g. fadd vs add.

The audience of this are people who are defining new target intrinsics.



================
Comment at: llvm/docs/LangRef.rst:10397-10401
+Intrinsic overloads based solely the distinction between whether the
+types are integer or floating point should not be relied upon for
+correct code generation. In such cases, the recommended approach when
+defining intrinsics is to create separate integer and FP intrinsics
+rather than rely on overloading.
----------------
arsenm wrote:
> I feel like this could be rephrased more generally as something along the lines of there shouldn't be a major semantic change in the operation based on the overloaded type. The integer/FP distinction is one particular example. I'm trying to come up with a definition for "major semantic change" means though.
> 
> I have a sort of counterexample though in the AMDGPU buffer intrinsics. If the return type is a struct, it changes the instruction slightly, but not in a problematic/ambiguous way like the FP/integer type.
Perhaps, but I think we should try to be specific.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59657