This is an archive of the discontinued LLVM Phabricator instance.

[Headers][doc] Add FMA intrinsic descriptions
ClosedPublic

Authored by probinson on Apr 11 2023, 8:00 AM.

Diff Detail

Event Timeline

probinson created this revision.Apr 11 2023, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2023, 8:00 AM
probinson requested review of this revision.Apr 11 2023, 8:00 AM
pengfei added inline comments.Apr 11 2023, 8:18 AM
clang/lib/Headers/fmaintrin.h
22

We are using a special format to describute the function in a pseudo code to share it with the intrinsic guide, e.g.,
https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/avx512fintrin.h#L9604-L9610
https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm512_i32logather_pd&ig_expand=4077

There's no strong requirement to follow it, but it would be better to adopt a uniform format.

26

It would be odd to user given this is just 1/3 instructions the intrinsic may generate, but I don't have a good idea here.

probinson added inline comments.Apr 13 2023, 8:37 AM
clang/lib/Headers/fmaintrin.h
22

Is a FOR loop with computed bit offsets really clearer than "For each element" ? Is it valuable to repeat information that can be found in the instruction reference?
I can accept answers of "yes" and "yes" because I am not someone who ever deals with vector data, but I would be a little surprised by those answers.

26

I listed the 213 version because that's the one that multiplies the first two operands, and the intrinsic multiplies the first two operands. So it's the instruction that most closely corresponds to the intrinsic.
We don't guarantee that the "corresponding" instruction is what is actually generated, in general. I know this point has come up before regarding intrinsic descriptions. My thinking is that the "corresponding instruction" gives the reader a place to look in the instruction reference manual, so listing only one is again okay.

pengfei accepted this revision.Apr 16 2023, 7:30 PM
pengfei added inline comments.
clang/lib/Headers/fmaintrin.h
22

We have internal tools to verify them according to these offsets (but of course not for 100% intrinsics), which means we can trust such information in most time.

The suggestion is based on:

  1. Correctness. It's easy to be errorprone for tedious documentations and we don't have other verifications but eyes;
  2. Unity. Using the same format is not only better for clean code but also distinct for future developers to follow;

As I said before, I'm not forcing this way. You decide it.

26

Note, intrinsics are force inlined. It's rare user will wrap it in a simple function with the same arguments order. In reality, each version may be generated. https://godbolt.org/z/q7j8Wxrnb

However, I'm not against this approach if we don't have any better way to describe it.

This revision is now accepted and ready to land.Apr 16 2023, 7:30 PM
This revision was landed with ongoing or failed builds.Apr 18 2023, 9:31 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2023, 9:31 AM

I chose to leave the "for each element" cases as-is, but I will keep your comments in mind as I go through other intrinsics.