Page MenuHomePhabricator

[DebugInfo] Add support for __fp16, float, and double constants.
ClosedPublic

Authored by dgross on Dec 7 2016, 3:07 PM.

Details

Summary

Partial fix for PR26619.

Prior to this change, a DIGlobalVariable corresponding to a static
const was marked with an expression corresponding to its constant
value only if it is of integral type. With this change, we now do the
same if it is of __fp16, float, or double type (that is,
floating-point types that do not exceed 64 bits in size, and hence are
supported easily by the existing LLVM machinery for creating constant
expressions in debug info).

Diff Detail

Repository
rL LLVM

Event Timeline

dgross updated this revision to Diff 80674.Dec 7 2016, 3:07 PM
dgross retitled this revision from to [DebugInfo] Add support for __fp16, float, and double constants..
dgross updated this object.
dgross added a reviewer: llvm-commits.
dgross added a comment.Dec 7 2016, 3:13 PM

See llvm regression test here: https://reviews.llvm.org/D27551

Hi David!
As this is a Clang patch, you should subscribe cfe-commits rather than llvm-commits. I've done that for you.
See also inline comments.

lib/CodeGen/CGDebugInfo.cpp
3765 ↗(On Diff #80674)

This line exceeds 80 columns. clang-format is your friend.

test/CodeGen/debug-info-static-const-fp.c
19 ↗(On Diff #80674)

Checking for the entire exact line for the variable is a bit fragile. Really what you want to do is associate the variable with the correct expression, and ignore all the irrelevant details. Something like this:

// CHECK: DIGlobalVariable(name: "hVal", {{.*}} expr: ![[HEXPR:{{[0-9]+}}]]
// CHECK: ![[HEXPR] = ! DIExpression(DW_OP_constu, 16502, DW_OP_stack_value)

And similar pairs for the other variables. I'd use a different FileCheck variable for each case.

dgross updated this revision to Diff 80683.Dec 7 2016, 3:57 PM

Incorporate code review feedback.

  • Reformat source.
  • Make test pattern more general.
dgross added inline comments.Dec 7 2016, 3:59 PM
lib/CodeGen/CGDebugInfo.cpp
3765 ↗(On Diff #80674)

Done. Is clang-format mentioned in any of the documentation that describes the development process? I might have missed it (not that that's an excuse for a needless violation of the coding guidelines).

test/CodeGen/debug-info-static-const-fp.c
19 ↗(On Diff #80674)

Thanks for the suggestion. I didn't realize FileCheck had variable support.

probinson accepted this revision.Dec 7 2016, 4:35 PM
probinson added a reviewer: probinson.

LGTM.

lib/CodeGen/CGDebugInfo.cpp
3765 ↗(On Diff #80674)

I would have thought the coding guidelines would mention clang-format, but I'm not finding it there (or in the stuff about submitting a patch).

This revision is now accepted and ready to land.Dec 7 2016, 4:35 PM
This revision was automatically updated to reflect the committed changes.