This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Fix LiveVariables error after optimizing VGPR ranges
ClosedPublic

Authored by arsenm on Jan 5 2022, 5:27 PM.

Details

Reviewers
ruiling
critson
foad
rampitec
Group Reviewers
Restricted Project
Summary

This was not removing the block from the live set depending on the
specific depth first visit order. Fixes a verifier error in the OpenCL
conformance tests.

Diff Detail

Event Timeline

arsenm created this revision.Jan 5 2022, 5:27 PM
arsenm requested review of this revision.Jan 5 2022, 5:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2022, 5:27 PM
Herald added a subscriber: wdng. · View Herald Transcript

It seems to cause a real regression, block-should-not-be-in-alive-blocks.mir fails.

ruiling added inline comments.Jan 7 2022, 6:41 AM
llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp
386–387

A pre-order walk over the blocks should more match the need here. the dfs walk does not work if we meet Flow block first, thus skipping some blocks in the THEN region. Using "continue" will make the traversal going all the way to the return block of the function.

ruiling added inline comments.Jan 7 2022, 5:22 PM
llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp
386–387

I take a second look, the order of the blocks does not matter here. We can use any order of traversal to collect all the blocks in the THEN region before this for-loop. We can use something like below to get all the blocks. (I choose SetVector mainly to keep the later iteration over the blocks is still in some kind of deterministic order. DenseSet also works.)

SetVector<MachineBasicBlock*> Blocks;
SmallVector<MachineBasicBlock *> WorkList;

WorkList.push_back(If);
while (!WorkList.empty()) {
  auto *MBB = WorkList.pop_back_val(); 
  for (auto *Succ : MBB->successors()) {
    if (Succ != Flow && !Blocks.contains(Succ)) {
      WorkList.push_back(Succ);
      Blocks.insert(Succ);
    }
  }
}
arsenm updated this revision to Diff 398450.Jan 9 2022, 11:35 AM

Use set and worklist

ruiling accepted this revision.Jan 10 2022, 6:35 AM

LGTM with one minor comment.

llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp
377–384

Could you also remove this piece of code for the ThenEntry? They are not needed anymore.

This revision is now accepted and ready to land.Jan 10 2022, 6:35 AM