[AMDGPU] Simplify getNumFlatOffsetBits. NFC.

[foad]

Summary

Previously we considered this field to be either N-bit unsigned or
N+1-bit signed, depending on the instruction. I think it's conceptually
simpler to say that the field is always N+1-bit signed, but some
instructions do not allow negative values.

[kosarev]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7968

Is this actually an improvement? Wouldn't it be conceptually simpler to pass FlatVariant and get the actual width and, if needed, signedness?

[foad]

Is this actually an improvement?

I think so (of course!) because it simplifies getNumFlatOffsetBits without making its callers more complicated.

Wouldn't it be conceptually simpler to pass FlatVariant and get the actual width and, if needed, signedness?

Not sure what you mean by "actual width" here. I would not want to revert to getNumFlatOffsetBits returning different values for different flat variants on the same subtarget. But I suppose an additional patch to make getNumFlatOffsetBits return the "AllowNegative" flag as well as the field width might be an improvement.

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7942

I don't really like the AllowNegative naming. SignBitIgnored?

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

2507–2510

Can return ternary operator

[kosarev]

By actual width I mean what can be passed to isIntN()/isUInt() without adjustments. The AllowNegative thing feels a bit alien to the ISA, effectively adding an extra concept while not eliminating the need for adjustments. Signedness, in contrast, looks rather natural and familiar, and if (Signed ? isIntN(...) : isUIntN(...)) is probably easier to understand than if (!isIntN(...) || (!AllowNegative && Op.getImm() < 0)).

[foad]

The AllowNegative thing feels a bit alien to the ISA, effectively adding an extra concept while not eliminating the need for adjustments.

Then I think we simply disagree about which one is conceptually simpler.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7942

I don't think "ignored" is right. In at least some cases (like hasNegativeScratchOffsetBug) it is definitely not ignored but leads to wrong behaviour.

[scott.linder]

In D140883#4023012, @foad wrote:

Is this actually an improvement?

I think so (of course!) because it simplifies getNumFlatOffsetBits without making its callers more complicated.

I agree that it is a bit simpler to conceptualize it as a signed field with a common size, especially as we already have to think about the "negative scratch offset bug" for some "signed" versions of the field. At that point, might as well just say it is always a signed field, and negative values are sometimes illegal.

Wouldn't it be conceptually simpler to pass FlatVariant and get the actual width and, if needed, signedness?

Not sure what you mean by "actual width" here. I would not want to revert to getNumFlatOffsetBits returning different values for different flat variants on the same subtarget. But I suppose an additional patch to make getNumFlatOffsetBits return the "AllowNegative" flag as well as the field width might be an improvement.

+1 to returning AllowNegative from getNumFlatOffsetBits, especially with structured bindings making it less awkward:

auto [OffsetSize, AllowNegative] = AMDGPU::getNumFlatOffsetBits(getSTI(), TSFlags);

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

4119

4120–4121

If the MSB is not always ignored, this comment should be updated/deleted

[kosarev]

I agree that it is a bit simpler to conceptualize it as a signed field with a common size, especially as we already have to think about the "negative scratch offset bug" for some "signed" versions of the field. At that point, might as well just say it is always a signed field, and negative values are sometimes illegal.

The advantage of the original code is that it encapsulates (apart from the signedness and bug-handling bits, sadly) these specifics in a single place, getNumFlatOffsetBits(), whereas the proposed version effectively lets them leak to the surrounding logic. So on generating the error particularly the new code translates the signed non-negatives back to the precision-and-signedness representation.

[foad]

So on generating the error particularly the new code translates the signed non-negatives back to the precision-and-signedness representation.

I only did that to make the patch NFC including the text of error messages. Really I'd prefer to reword the error message too, to something like "expected an N-bit signed offset" or "expected a non-negative N-bit signed offset".

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

4120–4121

In the absence of hardware bugs this might be true; I really don't know. I will see if I can find some good documentation.

OTOH it is not a very useful comment for us, since the compiler never needs to rely on the bit being "ignored and forced to zero".

[kosarev]

"expected a non-negative N-bit signed offset"

Might be worth ruminating on whether that would be an improvement as well before moving forward with this patch, on which, apart from noting that silicon-level bugs are probably not the best reasoning for user-exposed conceptual models, I've decided to have no opinion. :)

Things to take into consideration: that the offset bug will retire at some point and the chance for the hardware to support other offset representations in the future.

[scott.linder]

In D140883#4025487, @kosarev wrote:

I agree that it is a bit simpler to conceptualize it as a signed field with a common size, especially as we already have to think about the "negative scratch offset bug" for some "signed" versions of the field. At that point, might as well just say it is always a signed field, and negative values are sometimes illegal.

The advantage of the original code is that it encapsulates (apart from the signedness and bug-handling bits, sadly) these specifics in a single place, getNumFlatOffsetBits(), whereas the proposed version effectively lets them leak to the surrounding logic. So on generating the error particularly the new code translates the signed non-negatives back to the precision-and-signedness representation.

I'm not sure I follow the argument here, as the only thing being encapsulated is the field size as a function of the signedness, but you immediately say the signedness is sadly not encapsulated. What is being encapsulated more completely in the old version?

In D140883#4028873, @kosarev wrote:

"expected a non-negative N-bit signed offset"

Might be worth ruminating on whether that would be an improvement as well before moving forward with this patch, on which, apart from noting that silicon-level bugs are probably not the best reasoning for user-exposed conceptual models, I've decided to have no opinion. :)

Things to take into consideration: that the offset bug will retire at some point and the chance for the hardware to support other offset representations in the future.

I think this is the most compelling argument for leaving things as they are, along with the fact that anything we change here adds a small gap between the compiler writer/user's model and the model that the hardware specs document.

[scott.linder]

In D140883#4029620, @scott.linder wrote:

In D140883#4025487, @kosarev wrote:

I agree that it is a bit simpler to conceptualize it as a signed field with a common size, especially as we already have to think about the "negative scratch offset bug" for some "signed" versions of the field. At that point, might as well just say it is always a signed field, and negative values are sometimes illegal.

The advantage of the original code is that it encapsulates (apart from the signedness and bug-handling bits, sadly) these specifics in a single place, getNumFlatOffsetBits(), whereas the proposed version effectively lets them leak to the surrounding logic. So on generating the error particularly the new code translates the signed non-negatives back to the precision-and-signedness representation.

I'm not sure I follow the argument here, as the only thing being encapsulated is the field size as a function of the signedness, but you immediately say the signedness is sadly not encapsulated. What is being encapsulated more completely in the old version?

In D140883#4028873, @kosarev wrote:

"expected a non-negative N-bit signed offset"

Might be worth ruminating on whether that would be an improvement as well before moving forward with this patch, on which, apart from noting that silicon-level bugs are probably not the best reasoning for user-exposed conceptual models, I've decided to have no opinion. :)

Things to take into consideration: that the offset bug will retire at some point and the chance for the hardware to support other offset representations in the future.

I think this is the most compelling argument for leaving things as they are, along with the fact that anything we change here adds a small gap between the compiler writer/user's model and the model that the hardware specs document.

Also, just to clarify, even in light of the arguments against it I still vote for the change being made.

[scott.linder]

LGTM

I'm open to renaming things, but I think AllowNegative is at least as clear as anything I can think of.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

bool AMDGPUAsmParser::validateFlatOffset(const MCInst &Inst,
 
   const auto &Op = Inst.getOperand(OpNum);
   if (!hasFlatOffsets() && Op.getImm() != 0) {
     Error(getFlatOffsetLoc(Operands),
           "flat offset modifier is not supported on this GPU");
     return false;
   }
 
   // For FLAT segment the offset must be positive;
   // MSB is ignored and forced to zero.
-  if (TSFlags & (SIInstrFlags::FlatGlobal | SIInstrFlags::FlatScratch)) {
-    unsigned OffsetSize = AMDGPU::getNumFlatOffsetBits(getSTI(), true);
-    if (!isIntN(OffsetSize, Op.getImm())) {
-      Error(getFlatOffsetLoc(Operands),
-            Twine("expected a ") + Twine(OffsetSize) + "-bit signed offset");
-      return false;
-    }
-  } else {
-    unsigned OffsetSize = AMDGPU::getNumFlatOffsetBits(getSTI(), false);
-    if (!isUIntN(OffsetSize, Op.getImm())) {
-      Error(getFlatOffsetLoc(Operands),
-            Twine("expected a ") + Twine(OffsetSize) + "-bit unsigned offset");
-      return false;
-    }
-  }
+  unsigned OffsetSize = AMDGPU::getNumFlatOffsetBits(getSTI());
+  bool AllowNegative =
+      TSFlags & (SIInstrFlags::FlatGlobal | SIInstrFlags::FlatScratch);
+  if (!isIntN(OffsetSize, Op.getImm()) || (!AllowNegative && Op.getImm() < 0)) {
+          Twine("expected a ") +
+              (AllowNegative ? Twine(OffsetSize) + "-bit signed offset"
+                             : Twine(OffsetSize - 1) + "-bit unsigned offset"));
 
   return true;
 }
 
 SMLoc AMDGPUAsmParser::getSMEMOffsetLoc(const OperandVector &Operands) const {
   // Start with second operand because SMEM Offset cannot be dst or src0.
   for (unsigned i = 2, e = Operands.size(); i != e; ++i) {
     AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[i]);

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

bool SIInstrInfo::isLegalFLATOffset(int64_t Offset, unsigned AddrSpace,
   if (!ST.hasFlatInstOffsets())
     return false;
 
   if (ST.hasFlatSegmentOffsetBug() && FlatVariant == SIInstrFlags::FLAT &&
       (AddrSpace == AMDGPUAS::FLAT_ADDRESS ||
        AddrSpace == AMDGPUAS::GLOBAL_ADDRESS))
     return false;
 
-  bool Signed = FlatVariant != SIInstrFlags::FLAT;
+  bool AllowNegative = FlatVariant != SIInstrFlags::FLAT;
   if (ST.hasNegativeScratchOffsetBug() &&
       FlatVariant == SIInstrFlags::FlatScratch)
-    Signed = false;
+    AllowNegative = false;
   if (ST.hasNegativeUnalignedScratchOffsetBug() &&
       FlatVariant == SIInstrFlags::FlatScratch && Offset < 0 &&
       (Offset % 4) != 0) {
     return false;
   }
 
-  unsigned N = AMDGPU::getNumFlatOffsetBits(ST, Signed);
-  return Signed ? isIntN(N, Offset) : isUIntN(N, Offset);
+  unsigned N = AMDGPU::getNumFlatOffsetBits(ST);
+  return isIntN(N, Offset) && (AllowNegative || Offset >= 0);
 }
 
 // See comment on SIInstrInfo::isLegalFLATOffset for what is legal and what not.
 std::pair<int64_t, int64_t>
 SIInstrInfo::splitFlatOffset(int64_t COffsetVal, unsigned AddrSpace,
                              uint64_t FlatVariant) const {
   int64_t RemainderOffset = COffsetVal;
   int64_t ImmField = 0;
-  bool Signed = FlatVariant != SIInstrFlags::FLAT;
+  bool AllowNegative = FlatVariant != SIInstrFlags::FLAT;
   if (ST.hasNegativeScratchOffsetBug() &&
       FlatVariant == SIInstrFlags::FlatScratch)
-    Signed = false;
+    AllowNegative = false;
 
-  const unsigned NumBits = AMDGPU::getNumFlatOffsetBits(ST, Signed);
-  if (Signed) {
+  const unsigned NumBits = AMDGPU::getNumFlatOffsetBits(ST) - 1;
+  if (AllowNegative) {
     // Use signed division by a power of two to truncate towards 0.
-    int64_t D = 1LL << (NumBits - 1);
+    int64_t D = 1LL << NumBits;
     RemainderOffset = (COffsetVal / D) * D;
     ImmField = COffsetVal - RemainderOffset;
 
     if (ST.hasNegativeUnalignedScratchOffsetBug() &&
         FlatVariant == SIInstrFlags::FlatScratch && ImmField < 0 &&
         (ImmField % 4) != 0) {
       // Make ImmField a multiple of 4
       RemainderOffset += ImmField % 4;

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

 /// \return The encoding that can be used for a 32-bit literal offset in an SMRD
 /// instruction. This is only useful on CI.s
 std::optional<int64_t> getSMRDEncodedLiteralOffset32(const MCSubtargetInfo &ST,
                                                      int64_t ByteOffset);
 
 /// For FLAT segment the offset must be positive;
 /// MSB is ignored and forced to zero.
 ///
-/// \return The number of bits available for the offset field in flat
-/// instructions.
-unsigned getNumFlatOffsetBits(const MCSubtargetInfo &ST, bool Signed);
+/// \return The number of bits available for the signed offset field in flat
+/// instructions. Note that some forms of the instruction disallow negative
+/// offsets.
+unsigned getNumFlatOffsetBits(const MCSubtargetInfo &ST);
 
 /// \returns true if this offset is small enough to fit in the SMRD
 /// offset field.  \p ByteOffset should be the offset in bytes and
 /// not the encoded offset.
 bool isLegalSMRDImmOffset(const MCSubtargetInfo &ST, int64_t ByteOffset);
 
 bool splitMUBUFOffset(uint32_t Imm, uint32_t &SOffset, uint32_t &ImmOffset,
                       const GCNSubtarget *Subtarget,

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

std::optional<int64_t> getSMRDEncodedLiteralOffset32(const MCSubtargetInfo &ST,
   if (!isCI(ST) || !isDwordAligned(ByteOffset))
     return std::nullopt;
 
   int64_t EncodedOffset = convertSMRDOffsetUnits(ST, ByteOffset);
   return isUInt<32>(EncodedOffset) ? std::optional<int64_t>(EncodedOffset)
                                    : std::nullopt;
 }
 
-unsigned getNumFlatOffsetBits(const MCSubtargetInfo &ST, bool Signed) {
+unsigned getNumFlatOffsetBits(const MCSubtargetInfo &ST) {
   // Address offset is 12-bit signed for GFX10, 13-bit for GFX9 and GFX11+.
   if (AMDGPU::isGFX10(ST))
-    return Signed ? 12 : 11;
+    return 12;
 
-  return Signed ? 13 : 12;
+  return 13;
 }
 
 // Given Imm, split it into the values to put into the SOffset and ImmOffset
 // fields in an MUBUF instruction. Return false if it is not possible (due to a
 // hardware bug needing a workaround).
 //
 // The required alignment ensures that individual address components remain
 // aligned if they are aligned to begin with. It also ensures that additional