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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155881