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

Tue Jan 10 08:04:37 PST 2023

scott.linder added a comment.
Herald added a subscriber: StephenFan.

In D140883#4029620 <https://reviews.llvm.org/D140883#4029620>, @scott.linder wrote:

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140883