This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine][debuginfo] Update valueCoversEntireFragment for fixed size fragment and scalable value.
ClosedPublic

Authored by fakepaper56 on Feb 21 2023, 4:06 AM.

Details

Summary

Update valueCoversEntireFragment to handle new case and add regression test.

Diff Detail

Event Timeline

fakepaper56 created this revision.Feb 21 2023, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 4:06 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
fakepaper56 requested review of this revision.Feb 21 2023, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 4:06 AM
sdesmalen added inline comments.Feb 21 2023, 4:17 AM
llvm/test/Transforms/InstCombine/dbg-scalable-store-fixed-frag.ll
2

There are no CHECK-lines and the running opt on this file doesn't cause an assertion failure for me. What is this test supposed to verify?

Update test case.

fakepaper56 added inline comments.Feb 21 2023, 7:19 AM
llvm/test/Transforms/InstCombine/dbg-scalable-store-fixed-frag.ll
2

Sorry that I update the test. It should fail by assertion.

sdesmalen added inline comments.Feb 23 2023, 1:01 AM
llvm/lib/Transforms/Utils/Local.cpp
1498–1499

I'm not sure how valueCoversEntireFragment is used, but would it make more sense to write:

return TypeSize::isKnownGE(ValueSize, TypeSize::getFixed(*FragmentSize)

?

llvm/test/Transforms/InstCombine/dbg-scalable-store-fixed-frag.ll
2

Thanks, I can confirm the test now fails for me.

Address sdesmalen comment and refine test cases.

fakepaper56 added inline comments.Feb 23 2023, 7:23 AM
llvm/lib/Transforms/Utils/Local.cpp
1498–1499

Good point. Thank your reminder. I also add tests for converting gdb.declare to gdb.value.

llvm/test/Transforms/InstCombine/dbg-scalable-store-fixed-frag.ll
2

Thank you for your check.

aprantl added inline comments.Feb 28 2023, 9:03 AM
llvm/test/Transforms/InstCombine/dbg-scalable-store-fixed-frag.ll
25

This IR isn't well-formed and a future verifier might reject it. Can you created two distinct DISubprograms and local variables for each of the functions?

Use distinct debug infromation for different items.

fakepaper56 marked an inline comment as done.Mar 1 2023, 4:00 PM
fakepaper56 added inline comments.
llvm/test/Transforms/InstCombine/dbg-scalable-store-fixed-frag.ll
25

Thank your good point. I have fixed it.

sdesmalen accepted this revision.Mar 2 2023, 1:11 AM

Seems fine to me, but please give @aprantl a chance to have another look at it as well before you land it.

This revision is now accepted and ready to land.Mar 2 2023, 1:11 AM
fakepaper56 marked an inline comment as done.

Rebase and ping @aprantl.

aprantl accepted this revision.Mar 6 2023, 5:19 PM