[PATCH] D155881: [AMDGPU] Remove std::optional from VOPD::ComponentProps. NFC.

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In D155881#4529385 <https://reviews.llvm.org/D155881#4529385>, @scott.linder wrote:

> Drive-by comment, but is the impetus for the change just around trivial-copy-constructibility? This is guaranteed in C++20 at least, via https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0602r4.html and on my machine I get it even when compiling for C++17 (as it appears it might have been deemed a "defect" against C++17). Is it really worth making the code less clear to ensure this for older standards, when it will become a moot point in not too long?
>
> If the issue is actually the size of the object then there seems to be marginal benefit anyway; on my x86_64 Linux machine `sizeof(ComponentProps)` goes from `16` to `12` with this patch (i.e. the `std::optional` discriminant effectively occupies `4` bytes, I assume because of `3` bytes of padding). A more effective approach to shrink the object would then be to use a smaller `unsigned` type; I don't know if the fact that `MCInstrInfo::NumOperands` is `public` is just some historical artifact or if it really is part of the interface to the type, but in the latter case this version drops the `sizeof(ComponentProps)` to `8` on my machine:
>
>   // Properties of VOPD components.
>   class ComponentProps {
>   private:
>     using NumOperandsT = decltype(MCInstrDesc::NumOperands);
>     NumOperandsT SrcOperandsNum = 0;
>     std::optional<NumOperandsT> MandatoryLiteralIdx;
>     bool HasSrc2Acc = false;
>
> Of course, with more fuss that size could be even lower, but I vastly prefer the (admittedly imperfect) `std::optional` to any other option when it is feasible. In this case there isn't much room for confusion or mistakes, but I'd still prefer the absolutely-obvious version.
>
> If there is some other issue I missed I'm interested in understanding it better!

At this point I constantly hit this in the gdb, and it is really performance critical class. As you said, there is not much room for confusion in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155881