[PATCH] D134641: [AMDGPU][Backend] Fix user-after-free in AMDGPUReleaseVGPRs::isLastVGPRUseVMEMStore

[Juan Manuel Martinez Caamaño via Phabricator via llvm-commits]

jmmartinez added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReleaseVGPRs.cpp:70
+      BlockVMEMStore.resize(MF.getNumBlockIDs());
+      for (auto &MBB : MF) {
+        BlockVMEMStore[MBB.getNumber()] = lastBlockVGPRUseIsVMEMStore(MBB);
----------------
arsenm wrote:
> I'd expect this to be the post_order loop
This loop sets the initialization value for each `MachineBasicBlock`. This loop does not take into account the predecessors/successors and it can be done in any order. Maybe the name `lastBlockVGPRUseIsVMEMStore` is misleading? Could be renamed to something such as `lastVGPRUseInstInBlockIsVMEMStore` or `initVGPRUseInstInBlockIsVMEMStore`.

The loop after this one is the one that propagates the values from the predecessors to every block and that must be done in reverse-post-order.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReleaseVGPRs.cpp:76
+      for (auto *MBB : reverse(PostOrder)) {
+        auto LastUseIsVMEMStore = BlockVMEMStore[MBB->getNumber()];
+        LastUseIsVMEMStore = LastUseIsVMEMStore ||
----------------
arsenm wrote:
> Just use bool?
BitVector's `operator[]` returns a `BitVector::reference` and not a `bool&`. `BitVector::reference` encapsulates a reference to a single bit.

Maybe it's worth to explicitely state the type `BitVector::reference LastUseIsVMEMStore = BlockVMEMStore[MBB->getNumber()];`. I think that `auto` looks confusing.

For context, here's the code of `BitVector::reference`:

```
   // Encapsulation of a single bit.
   class reference {
  
     BitWord *WordRef;
     unsigned BitPos;
  
   public:
     reference(BitVector &b, unsigned Idx) {
       WordRef = &b.Bits[Idx / BITWORD_SIZE];
       BitPos = Idx % BITWORD_SIZE;
     }
  
     reference() = delete;
     reference(const reference&) = default;
  
     reference &operator=(reference t) {
       *this = bool(t);
       return *this;
     }
  
     reference& operator=(bool t) {
       if (t)
         *WordRef |= BitWord(1) << BitPos;
       else
         *WordRef &= ~(BitWord(1) << BitPos);
       return *this;
     }
  
     operator bool() const {
       return ((*WordRef) & (BitWord(1) << BitPos)) != 0;
     }
   };
```


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReleaseVGPRs.cpp:77
+        auto LastUseIsVMEMStore = BlockVMEMStore[MBB->getNumber()];
+        LastUseIsVMEMStore = LastUseIsVMEMStore ||
+                             any_of(MBB->predecessors(),
----------------
arsenm wrote:
> This looks like a dead variable?
Addressed in the previous comment, since it's not a reference it's not dead. But it surely looks confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134641