This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Fix dbg_value handling when forming soft clause bundles
ClosedPublic

Authored by arsenm on Jan 30 2021, 2:16 PM.

Details

Summary

DBG_VALUES placed between memory instructions would change
codegen. Skip over these and re-insert them after the bundle instead
of giving up on bundling.

Diff Detail

Event Timeline

arsenm created this revision.Jan 30 2021, 2:16 PM
arsenm requested review of this revision.Jan 30 2021, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2021, 2:16 PM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm updated this revision to Diff 320357.Jan 31 2021, 8:14 AM

Forgot to git add test

dfukalov added inline comments.Jan 31 2021, 3:52 PM
llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp
388

Why don't just create DbgInstrs here?

llvm/test/CodeGen/AMDGPU/soft-clause-dbg-value.mir
2

Am I right that we need phi-node-elimination, before to remove IsSSA property to avoid verifier' fail?
Or, dummy copy of first instruction %0:sreg_64 = COPY $sgpr4_sgpr5 ; defeat IsSSA detection can be added.

scott.linder added inline comments.Feb 1 2021, 10:29 AM
llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp
326

Since https://reviews.llvm.org/D92522, if you don't have a strong reason for choosing 8

390–392

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?

rampitec added inline comments.Feb 1 2021, 10:33 AM
llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp
388

Right, it can be local to this block and cleared/destroyed after re-insertion. It will take less memory.

arsenm updated this revision to Diff 320605.Feb 1 2021, 2:31 PM

Address comments

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp
388

This avoids reallocating on subsequent bundles if it's necessary

390–392

It doesn't make a difference

llvm/test/CodeGen/AMDGPU/soft-clause-dbg-value.mir
2

No. The pass cleared properties now clears SSA

rampitec accepted this revision.Feb 1 2021, 3:21 PM
This revision is now accepted and ready to land.Feb 1 2021, 3:21 PM
scott.linder added inline comments.Feb 2 2021, 8:39 AM
llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp
390–392

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?

arsenm added inline comments.Feb 2 2021, 8:47 AM
llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp
390–392

BundleNext is needed for incrementing the inner loop up to Next

scott.linder added inline comments.Feb 2 2021, 2:17 PM
llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp
388

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?

390–392

Thank you for changing this in the committed version!