[PATCH] D124246: [AMDGPU] Adjust wave priority based on VMEM instructions to avoid duty-cycling.

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:41
+  MBBInfo &operator[](const MachineBasicBlock *MBB) {
+    MBBInfo *&info = Infos[MBB];
+    if (!info)
----------------
If you keep this code, variables names should use CamelCase like `Info`.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:42
+
+class MBBInfoSet {
+public:
----------------
kosarev wrote:
> foad wrote:
> > foad wrote:
> > > kosarev wrote:
> > > > kosarev wrote:
> > > > > foad wrote:
> > > > > > kosarev wrote:
> > > > > > > foad wrote:
> > > > > > > > This seems like overkill. Can you just use a `DenseMap<const MachineBasicBlock *, bool>` directly? Or maybe just a `DenseSet<const MachineBasicBlock *>`?
> > > > > > > On the other hand, if we know we will need more fields there to count VALUs, then it's probably not worth to rewrite it back and forth? (I don't mind either way.)
> > > > > > > if we know we will need more fields there to count VALUs
> > > > > > 
> > > > > > I don't know that :)
> > > > > > 
> > > > > > In any case, why do you use a map of pointers to MBBInfo, instead of just a map of MBBInfo? That seems wasteful.
> > > > > I think the main desire was to have references to `MBBInfo` remain valid on insertions but also avoid the risk of `DenseMap` be dealing with what may potentially become values of larger size. Will give it another thought on Monday. : )
> > > > So if I'm not wrong that `DenseMap<>` may reallocate values on insertion, that means some rather innocently looking things like `MBBInfos[A].NumSth + MBBInfos[B].NumSth` being outlawed, and they are probably not the easiest things to catch during preparing changes or review.
> > > > 
> > > > If we see these few extra lines preventing the risk a complication beyond immediate needs, then I don't mind removing them, but do we really think it's worth it?
> > > I would prefer not to have the extra complication. Other reviewers might disagree.
> > Actually I don't understand your argument. If `MBBInfos[A].NumSth + MBBInfos[B].NumSth` is not safe, then how does adding a level of indirection and writing `MBBInfos[A]->NumSth + MBBInfos[B]->NumSth` make it any safer?
> Not sure I understand how levels of indirection are relevant, but if we still compare using the `MBBInfoSet::operator[]` above with using `DenseMap::operator[]` directly, then I think what makes the first safer is that it doesn't return references to reallocatable storage. Meaning in `MBBInfos[A].NumSth + MBBInfos[B].NumSth` we don't depend on the order of calling the `MBBInfoSet::operator[]`s and the structure dereferences.
OK, I understand now. Sorry I had missed (or forgotten) that you had written your own operator[].

Personally I still prefer not to have the extra complication.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124246