This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)
ClosedPublic

Authored by bjope on May 1 2018, 12:51 PM.

Details

Summary

This is a follow up to rL331182. A PHI node can be split up into
several MIR PHI nodes when being selected. When there is a
dbg.value intrinsic that uses the result of such a PHI node we
need to select several DBG_VALUE instructions, with fragment
expressions, in order to do a correct selection.

Diff Detail

Repository
rL LLVM

Event Timeline

bjope created this revision.May 1 2018, 12:51 PM
mattd added a subscriber: mattd.May 1 2018, 1:12 PM
vsk added inline comments.May 1 2018, 1:13 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5267 ↗(On Diff #144767)

Would it be more convenient to add a 'occupiesMultipleRegs()' predicate to RegsForValue?

5273 ↗(On Diff #144767)

You might be able to simplify this by using zip_first(RegCount, RegVTs) (see STLExtras.h, or unittests/ADT/IteratorTest for example usage).

bjope added inline comments.May 1 2018, 1:32 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5273 ↗(On Diff #144767)

I basically stole this code/loop from EmitFuncArgumentDbgValue (line ~4917). But your suggestion sounds like a really good idea.

May I suggest a follow up commit to refactor this and the code in EmitFuncArgumentDbgValue at the same time? Or is it preferred to have this one nice-looking from the start? (I can totally but that...)

vsk added inline comments.May 1 2018, 1:43 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5273 ↗(On Diff #144767)

I'd be fine with a follow-up commit to clean up both sites.

Although now that you mention this, there may be a opportunity here to share code. It's conceivable that some common helper could take care of creating the fragment expressions, and then passing {fragmentExpr, Offset, Reg} to a lambda.

test/DebugInfo/X86/sdag-dbgvalue-phi-use-3.ll
123 ↗(On Diff #144767)

Consider stripping out these attributes, lifetime markers, and tbaa metadata to make the test more maintainable.

bjope updated this revision to Diff 144786.May 1 2018, 2:14 PM

Some updates suggested by vsk:

  • Use zip_first.
  • Add a RegsForValue::occupiesMultipleRegisters() helper.
  • Cleanup in the test case (removing tbaa and lifetime stuff).

I'll create another patch to do a similar refactoring in EmitFuncArgumentDbgValue (or see if we can share code in some way).

bjope marked 3 inline comments as done.May 1 2018, 2:17 PM
vsk accepted this revision.May 1 2018, 2:44 PM

Looks good to me. Thanks!

This revision is now accepted and ready to land.May 1 2018, 2:44 PM
This revision was automatically updated to reflect the committed changes.

Unfortunately, this broke things for me, see https://bugs.llvm.org/show_bug.cgi?id=37321 for details. I'll revert this one in the meantime.

bjope added a comment.May 3 2018, 12:25 AM

Unfortunately, this broke things for me, see https://bugs.llvm.org/show_bug.cgi?id=37321 for details. I'll revert this one in the meantime.

Ok. I'll take a look at it!