[PATCH] D140883: [AMDGPU] Simplify getNumFlatOffsetBits. NFC.

Tue Jan 3 18:29:17 PST 2023

scott.linder added a comment.

In D140883#4023012 <https://reviews.llvm.org/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);

================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:4121

   // For FLAT segment the offset must be positive;
   // MSB is ignored and forced to zero.
----------------

================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:4122
   // For FLAT segment the offset must be positive;
   // MSB is ignored and forced to zero.
+  unsigned OffsetSize = AMDGPU::getNumFlatOffsetBits(getSTI());
----------------
If the MSB is not always ignored, this comment should be updated/deleted

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140883