Page MenuHomePhabricator

Debug info: Teach the SDag type legalizer how to split up debug info for integer values that are split into a hi and lo part.
AbandonedPublic

Authored by aprantl on Aug 8 2014, 1:07 PM.

Details

Reviewers
None
Summary

Teach the SDag type legalizer how to split up debug info for integer values that are split into a hi and lo part.

This is yet another a split-off from http://reviews.llvm.org/D2680

Diff Detail

Event Timeline

aprantl updated this revision to Diff 12312.Aug 8 2014, 1:07 PM
aprantl retitled this revision from to Debug info: Teach the SDag type legalizer how to split up debug info for integer values that are split into a hi and lo part..
aprantl updated this object.
aprantl edited the test plan for this revision. (Show Details)
aprantl added subscribers: echristo, dblaikie, chandlerc.
aprantl added a subscriber: Unknown Object (MLST).Aug 8 2014, 1:25 PM
aprantl abandoned this revision.Aug 8 2014, 2:16 PM

Closing in favor of D4831.

Closed the wrong one :) Hopefully you can reopen this one. Sorry for
all the churn/confusion/annoyance... :/

dblaikie added inline comments.Aug 8 2014, 2:23 PM
lib/CodeGen/SelectionDAG/InstrEmitter.cpp
675

Not knowing much about any of this SelDAG code, I don't understand how this change is related to the rest of the patch - could you explain it?

test/DebugInfo/X86/sdagsplit-1.ll
2

Same feedback as the other test case's RUN line.

35

I'm a little confused as to how the use of %add comes before its definition... - but I guess that's working as intended somehow?

test/DebugInfo/X86/sdagsplit-2.ll
1

Not sure about your preference, but I usually avoid the temporary file and just pipe from one to the next:

; RUN: %llc_dwarf -filetype=obj < %s | FileCheck %s

Note also the use of %llc_dwarf (I keep forgetting why we need that, and why it's not done more consistently - but something to do with Windows support... Takumi knows more about this) and < %s, so that the output doesn't risk containing the filename of the input (which might vary depending on test configuration/directory hierarchy, and thus fail on some machines and pass on others).

2

Why the custom check prefix rather than just the default CHECK?

10

Any particular reason this needs 3 parameters and a return value? (are they testing different codepaths? If they're all testing the same one, could we just use a single parameter?)

15

I see the return value is the only difference between these two test cases. Is it relevant? (I assume so, - guess that's how you ended up with two test cases) Could it be documented in the test better?

Is the functionality that makes this test pass separate from the functionality that makes the first test pass - if so, perhaps it could be separated further. I'm still muddling through the actual source changes & trying to figure out how they're all dependent/related.

I _think_ I just messed up the comment.
I closed http://reviews.llvm.org/D4831 and http://reviews.llvm.org/D4832 is still open.

Ah, right -it's all getting jumbled up in my email because it has the
same subject line (Go GMail!)

I sent review to the wrong one - will rewrite/repost shortly. Blagle.

aprantl reclaimed this revision.Oct 3 2014, 5:29 PM
aprantl planned changes to this revision.
aprantl abandoned this revision.

Is there any way to close a phabricator revision without accepting it?

Yep, you "Abandon" it (if you own the CR you can choose "Abandon" as one of
the actions).

Yes, but now that I abandoned it, it's still lying around. Can't I close or delete it now?

Ah. Not sure - it can be "closed". I'm not sure if "closed" implies
successful resolution or not... doesn't seem like a bad thing to close a
revision even if it was never committed, but I don't really know.

Yes, but now that I abandoned it, it's still lying around. Can't I close
or delete it now?

http://reviews.llvm.org/D4831

When you figure this out, please make an appropriate note in the Phab doc.
http://llvm.org/docs/Phabricator.html

Thanks,
--paulr
who learned that Phab hated him long ago, but would like to get along with it better