[PATCH] D96906: [AMDGPU] gfx90a support

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

In D96906#2572842 <https://reviews.llvm.org/D96906#2572842>, @msearles wrote:

> In D96906#2572749 <https://reviews.llvm.org/D96906#2572749>, @kzhuravl wrote:
>
>>> The point is that nobody upstream even got a chance to chime in.
>>
>> We are and will be taking care of any feedback provided in this review post-commit.
>
> To be fair to @rampitec , it was not his desire to push this up in 1 big patch. We needed this upstreamed and no time was given to him to break it up into reasonably sized pieces. If it appears to be his doing/his intent, well, it should not. There have been a couple comments; I believe most addressed; comments will continue to be addressed.

Who landed the change is not particularly important.

What does matter is to make sure that shortcutting the review does not become a regular occurrence.

Stuff happens, Sometimes the standard rules may need to be bent. However, it should not be a unilateral decision by the committing side.

A better way to handle that would be to send the patch for review few days early (you presumably did have most/all of these changes made by then), provide details describing the changes (single subject line falls a bit short), outline your situation explaining why the patch can't be split and reviewed pre-commit. If the changes are indeed well-reviewed downstream, that would probably not pose much of a challenge to get the patch approved.  If the patch does need further cleanups, at least we would have a reasonable idea how invasive they would be and could make an informed call. "Commit now, ask for forgiveness later" is not among the LLVM contribution guidelines, not for large patches. At the very minimum it should've been publicly discussed before the fact.



================
Comment at: clang/include/clang/Driver/Options.td:3097-3101
+def mtgsplit : Flag<["-"], "mtgsplit">, Group<m_amdgpu_Features_Group>,
+  HelpText<"Enable threadgroup split execution mode (AMDGPU only)">;
+def mno_tgsplit : Flag<["-"], "mno-tgsplit">, Group<m_amdgpu_Features_Group>,
+  HelpText<"Disable threadgroup split execution mode (AMDGPU only)">;
+
----------------
We have `BoolFOption` to generate `-fsomething` and `-fno-something`


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:737
   EF_AMDGPU_MACH_AMDGCN_FIRST = EF_AMDGPU_MACH_AMDGCN_GFX600,
-  EF_AMDGPU_MACH_AMDGCN_LAST = EF_AMDGPU_MACH_AMDGCN_GFX805,
+  EF_AMDGPU_MACH_AMDGCN_LAST = EF_AMDGPU_MACH_AMDGCN_GFX90A,
 
----------------
Nit: This looks odd. GFX90A does not need to be in the middle of the list. It makes it somewhat confusing to tell which ID is really the last. The `_LAST` enum says it's GFX90A, but it's not the last item of the list. There are already out-of-name-order GPUs at the end of the list, so putting 90A at the end would probably be a better choice. At least we'd still have the numeric values in order. Right now the list is ordered neither by the name nor by the value.

There's also a question of whether something needs to be done about the missing values 0x3c..0x3e. Presumably the `_FIRST`..`_LAST` enums specify the range we'll use to iterate over the GPU IDs. Do we handle the missing values correctly?

Looks like it's benign at the moment as we're only using it to return amdgcn triple in ELFObjectFile.h. I'd add the placeholder enums for the reserved/unused values within the range.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96906