Page MenuHomePhabricator

[PredicateInfo] Use custom mangling to support ssa_copy with unnamed types.
ClosedPublic

Authored by fhahn on Jul 10 2018, 3:53 AM.

Details

Summary

This is a workaround and it would be better to fix this generally, but
doing it generally is quite tricky. See D48541 and PR38117.

Doing it in PredicateInfo directly allows us to use the type address to
differentiate different unnamed types, because neither the created
declarations nor the ssa_copy calls should be visible after
PredicateInfo got destroyed.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Jul 10 2018, 3:53 AM

If you're going to do this, might as well just replace the whole implementation getMangledTypeStr with "return utostr((uintptr_t)Ty);".

include/llvm/Transforms/Utils/PredicateInfo.h
266 ↗(On Diff #154771)

AssertingVH.

fhahn updated this revision to Diff 154881.Jul 10 2018, 2:19 PM

Thanks Eli! I am not really happy with the solution, but as discussed in D48541 solving that generally seems quite an invasive change.

I also thought about bitcasting the argument and return type of the intrinsic to and from i8* (as you suggested as another alternative), but this makes the IR more verbose, as we would have to use casts for all struct types/pointers to structs as they could contain unnamed types at any level and it would make life harder for users of PredicateInfo.

fhahn added inline comments.Jul 10 2018, 2:20 PM
include/llvm/Transforms/Utils/PredicateInfo.h
266 ↗(On Diff #154771)

Will do tomorrow, I think SmallSet needs support for iterating first. I have a patch for that, but it is not committed yet, as there was a problem with GCC on some bots having problems with is_trivially_copy_constructible & co

fhahn updated this revision to Diff 155024.Jul 11 2018, 9:43 AM

Use SmallSet + AssertingVH

efriedma accepted this revision.Jul 11 2018, 11:47 AM

LGTM with one nit. I'm not really happy we're creating temporarily invalid IR, but it shouldn't cause any problems, and I don't see a better solution here.

lib/Transforms/Utils/PredicateInfo.cpp
729 ↗(On Diff #155024)

Can you just use a SmallVector here?

This revision is now accepted and ready to land.Jul 11 2018, 11:47 AM
fhahn added a comment.Jul 16 2018, 1:36 PM

Thank you very much Eli! There was a problem with the SmallSet iterator because of MSVC STL's std::set iterator not being trivially copy constructible. I'll commit this once I manged to resolve the SmallSet iterator issue.

This revision was automatically updated to reflect the committed changes.