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

[rampitec]

Summary

This class has to be fast and efficient with a trivial copy
constructor.

[Joe_Nash]

LGTM

[scott.linder]

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!

[rampitec]

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

[arsenm]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

575–576

Can these be uint8_t (or even a packed uint8_t?)

[rampitec]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

575–576

We are not really saving space here, and I will have to add more fields later. But then:

SrcOperandsNum - 2 bits
MandatoryLiteralIdx - 4 bits
HasSrc2Acc - 1 bit

I.e. 7 bits total.

[scott.linder]

In D155881#4529400, @rampitec wrote:

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

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

 
 enum ComponentIndex : unsigned { X = 0, Y = 1 };
 constexpr unsigned COMPONENTS[] = {ComponentIndex::X, ComponentIndex::Y};
 constexpr unsigned COMPONENTS_NUM = 2;
 
 // Properties of VOPD components.
 class ComponentProps {
 private:
   unsigned SrcOperandsNum = 0;
-  std::optional<unsigned> MandatoryLiteralIdx;
+  unsigned MandatoryLiteralIdx = ~0u;
   bool HasSrc2Acc = false;
 
 public:
   ComponentProps() = default;
   ComponentProps(const MCInstrDesc &OpDesc);
 
   // Return the total number of src operands this component has.
   unsigned getCompSrcOperandsNum() const { return SrcOperandsNum; }
 
   // Return the number of src operands of this component visible to the parser.
   unsigned getCompParsedSrcOperandsNum() const {
     return SrcOperandsNum - HasSrc2Acc;
   }
 
   // Return true iif this component has a mandatory literal.
-  bool hasMandatoryLiteral() const { return MandatoryLiteralIdx.has_value(); }
+  bool hasMandatoryLiteral() const { return MandatoryLiteralIdx != ~0u; }
 
   // If this component has a mandatory literal, return component operand
   // index of this literal (i.e. either Component::SRC1 or Component::SRC2).
   unsigned getMandatoryLiteralCompOperandIndex() const {
     assert(hasMandatoryLiteral());
-    return *MandatoryLiteralIdx;
+    return MandatoryLiteralIdx;
   }
 
   // Return true iif this component has operand
   // with component index CompSrcIdx and this operand may be a register.
   bool hasRegSrcOperand(unsigned CompSrcIdx) const {
     assert(CompSrcIdx < Component::MAX_SRC_NUM);
     return SrcOperandsNum > CompSrcIdx && !hasMandatoryLiteralAt(CompSrcIdx);
   }
 
   // Return true iif this component has tied src2.
   bool hasSrc2Acc() const { return HasSrc2Acc; }
 
 private:
   bool hasMandatoryLiteralAt(unsigned CompSrcIdx) const {
     assert(CompSrcIdx < Component::MAX_SRC_NUM);
-    return hasMandatoryLiteral() &&
-           *MandatoryLiteralIdx == Component::DST_NUM + CompSrcIdx;
+    return MandatoryLiteralIdx == Component::DST_NUM + CompSrcIdx;
   }
 };
 
 enum ComponentKind : unsigned {
   SINGLE = 0,  // A single VOP1 or VOP2 instruction which may be used in VOPD.
   COMPONENT_X, // A VOPD instruction, X component.
   COMPONENT_Y, // A VOPD instruction, Y component.
   MAX = COMPONENT_Y