This is an archive of the discontinued LLVM Phabricator instance.

Clean up usages of asserting vector getters in Type
ClosedPublic

Authored by ctetreau on Apr 1 2020, 4:59 PM.

Details

Summary

Remove usages of asserting vector getters in Type in preparation for the
VectorType refactor. The existence of these functions complicates the
refactor while adding little value.

Diff Detail

Event Timeline

ctetreau created this revision.Apr 1 2020, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2020, 4:59 PM
ctetreau updated this revision to Diff 254614.Apr 2 2020, 1:50 PM

Update to note that this is NFC

ctetreau updated this revision to Diff 255849.Apr 7 2020, 4:07 PM

rebase, mention codegen in commit message

sdesmalen added inline comments.Apr 10 2020, 7:24 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
85–86

Can we just use cast<> and get rid of the separate assert below? We'd lose the message, but I'm not sure how useful that is, given that the context makes it quite obvious.

104

same here.

629

nit: Move this line closer to definition on 633?

ctetreau marked an inline comment as done.Apr 10 2020, 9:14 AM
ctetreau added inline comments.
llvm/include/llvm/CodeGen/BasicTTIImpl.h
85–86

I spoke to this a bit in D77259.

Realistically, for this example, in debug you'll get a slightly more readable assert message. In release, you'll get a null dereference in getVectorInstrCost vs (I assume) a template error in the cast. From a debugging perspective, the second situation is almost certainly preferable, but just doing the cast will be better for performance. I'll fix it.

ctetreau updated this revision to Diff 256587.Apr 10 2020, 9:21 AM

address code review issues

sdesmalen accepted this revision.Apr 10 2020, 1:28 PM

LGTM!

llvm/include/llvm/CodeGen/BasicTTIImpl.h
85–86

Thanks! In this case the only difference for a release build would be that VTy would be nullptr when using dyn_cast, and would be Ty when using cast. I don't think the use of cast gives a template error.

This revision is now accepted and ready to land.Apr 10 2020, 1:28 PM
ctetreau marked an inline comment as done.Apr 10 2020, 1:47 PM
ctetreau added inline comments.
llvm/include/llvm/CodeGen/BasicTTIImpl.h
85–86

I guess in a just world, you'd get a "types unrelated" error. But since it just does a c-style cast, it's probably just UB.

This revision was automatically updated to reflect the committed changes.