[PATCH] D155881: [AMDGPU] Remove std::optional from VOPD::ComponentProps. NFC.
Scott Linder via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 25 12:23:50 PDT 2023
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
More information about the llvm-commits
mailing list