Page MenuHomePhabricator

[IR] Remove CastInst::isCastable since it is not used
ClosedPublic

Authored by c-rhodes on Dec 3 2020, 4:37 AM.

Details

Summary

It was removed back in 2013 (f63dfbb) by Matt Arsenault but then
reverted since DragonEgg used it, but that project is no longer
maintained.

Can this be removed now?

Diff Detail

Event Timeline

c-rhodes created this revision.Dec 3 2020, 4:37 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 3 2020, 4:37 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
c-rhodes requested review of this revision.Dec 3 2020, 4:37 AM
ldionne accepted this revision as: Restricted Project.Dec 3 2020, 7:46 AM
ldionne added a subscriber: ldionne.

The libc++abi part is fine.

dexonsmith added inline comments.Dec 3 2020, 10:59 AM
libcxxabi/test/test_demangle.pass.cpp
21547 ↗(On Diff #309227)

I don't understand why this is being removed. Is the demangler testcase no longer valid?

ldionne added inline comments.Dec 3 2020, 11:38 AM
libcxxabi/test/test_demangle.pass.cpp
21547 ↗(On Diff #309227)

It's still valid -- the only reason why I'm not opposing to the removal is that I can see a *small* benefit in not leaving behind old definitions like that, which show up whenever you grep for the function name in the whole monorepo.

Basically, I don't mind whether we keep it or not. I think the benefit is small, but the loss of test coverage is also very small, since I assume those symbols were just taken more or less randomly and passed through c++filt.

dexonsmith added inline comments.Dec 3 2020, 11:51 AM
libcxxabi/test/test_demangle.pass.cpp
21547 ↗(On Diff #309227)

Yeah, I would have to guess that all the symbols in an LLVM binary were demangled with our previous system demangler, to add as testcases for libc++abi.

My inclination is that it's better to leave it behind, until / unless the demangler tests are rethought as a whole in some way (to get similar coverage another way).

I agree that this file coming up in git grep is a bit annoying (it happens a lot to me)... but since this patch deletes CastInst::isCastable I expect this line won't bother anyone again (since no one will be looking for the deleted function). It's all the other lines that will continue to cause friction.

I also worry that if we start deleting this one at a time, we'll add a bunch of uninteresting commits to:

% git log -- libcxxabi/

WDYT?

ldionne accepted this revision.Dec 3 2020, 11:53 AM
ldionne added inline comments.
libcxxabi/test/test_demangle.pass.cpp
21547 ↗(On Diff #309227)

Ok, I'm fine with your suggestion. @c-rhodes , can you please leave that test in libc++abi? And then libc++abi won't be affected by this change, so you still have my LGTM :-).

This revision is now accepted and ready to land.Dec 3 2020, 11:53 AM
dexonsmith accepted this revision.Dec 3 2020, 12:01 PM

LGTM too if you drop the libcxxabi change.

If there's an out-of-tree user that needs this we can revert again if it makes sense to (in which case, it'd be better to change getCastOpcode to return an Optional<Instruction::CastOps>, so we're not having to keep two functions in sync like this...).

c-rhodes updated this revision to Diff 309498.Dec 4 2020, 2:52 AM

Remove change to libcxxabi test.

c-rhodes marked an inline comment as done.Dec 4 2020, 3:00 AM

LGTM too if you drop the libcxxabi change.

If there's an out-of-tree user that needs this we can revert again if it makes sense to (in which case, it'd be better to change getCastOpcode to return an Optional<Instruction::CastOps>, so we're not having to keep two functions in sync like this...).

SGTM, I'll land it early next week in-case of any fallout, thanks for reviewing.

libcxxabi/test/test_demangle.pass.cpp
21547 ↗(On Diff #309227)

Ok, I'm fine with your suggestion. @c-rhodes , can you please leave that test in libc++abi? And then libc++abi won't be affected by this change, so you still have my LGTM :-).

np cheers, I've removed the change.

This revision was automatically updated to reflect the committed changes.