[PATCH] D95748: AMDGPU: Fix dbg_value handling when forming soft clause bundles
Scott Linder via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 2 14:17:44 PST 2021
scott.linder added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp:388
RPT.reset(*B, &LiveRegsCopy);
+ DbgInstrs.clear();
+
----------------
arsenm wrote:
> rampitec wrote:
> > dfukalov wrote:
> > > Why don't just create DbgInstrs here?
> > Right, it can be local to this block and cleared/destroyed after re-insertion. It will take less memory.
> This avoids reallocating on subsequent bundles if it's necessary
I didn't notice this when I first read, but it doesn't really seem necessary to buffer up the debug instrs at all. Can't we just `MBB.insert(Next, BI->removeFromParent())` below? Did you do the buffering and extra loop because it makes tracking when to end iteration simpler, and because the largest the vector can get is `amdgpu-max-memory-clause` anyway?
================
Comment at: llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp:390-392
+ auto BundleNext = I;
+ for (auto BI = I; BI != Next; BI = BundleNext) {
+ BundleNext = std::next(BI);
----------------
arsenm wrote:
> scott.linder wrote:
> > arsenm wrote:
> > > scott.linder wrote:
> > > > When does `Next != BundleNext` after this `for` terminates? It seems like you should be able to just use `Next` below when re-inserting the DBG instructions?
> > > It doesn't make a difference
> > I disagree that it doesn't matter, after reading to this point I have to assume either (a) I don't understand the code, or (b) there is a missing `assert(BundleNext == Next);`
> >
> > Can you either delete `BundleNext` or add an assert?
> BundleNext is needed for incrementing the inner loop up to Next
Thank you for changing this in the committed version!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95748/new/
https://reviews.llvm.org/D95748
More information about the llvm-commits
mailing list