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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In D155881#4529400 <https://reviews.llvm.org/D155881#4529400>, @rampitec wrote:

> 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.

Is there a significant performance cost in this case? We always check the sentinel/discriminant before accessing it anyway. If the issue is more around debuggability I don't have a silver bullet, I definitely wish I could do better than just GDB pretty-printers, as accessing members of an optional in a RelWithDebugInfo build is definitely tedious, but that's true of every STL type to some extent.

Anyway, there are plenty of issues with std::optional; as much as I would really love a nice monadic abstraction to make intent crystal clear everywhere I accept it just isn't worth it a lot of the time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155881