This is an archive of the discontinued LLVM Phabricator instance.

Small improvements to Intrinsic::getName
ClosedPublic

Authored by lxfind on Dec 2 2020, 4:35 PM.

Details

Summary

While I was adding a new intrinsic instruction (not overloaded), I accidentally used CreateUnaryIntrinsic to create the intrinsics, which turns out to be passing the type list to getName, and ended up naming the intrinsics function with type suffix, which leads to wierd bugs latter on. It took me a long time to debug.
It seems a good idea to add an assertion in getName so that it fails if types are passed but it's not a overloaded function.
Also, the overloade version of getName is less efficient because it creates an std::string. We should avoid calling it if we know that there are no types provided.

Diff Detail

Event Timeline

lxfind created this revision.Dec 2 2020, 4:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 4:35 PM
lxfind requested review of this revision.Dec 2 2020, 4:35 PM
lxfind added a reviewer: pete.Dec 2 2020, 4:38 PM
pete accepted this revision.Dec 2 2020, 4:44 PM

LGTM. Thanks for doing this!

This revision is now accepted and ready to land.Dec 2 2020, 4:44 PM
This revision was landed with ongoing or failed builds.Dec 2 2020, 4:49 PM
This revision was automatically updated to reflect the committed changes.
erichkeane added a subscriber: erichkeane.EditedJan 4 2021, 10:16 AM

I note here: https://llvm.org/doxygen/namespacellvm_1_1Intrinsic.html#a7157f9fa9dd11f234ec3c58517cb6d96 that the documentatoin to the getName function variant says:

"If no overloads are requried, it is safe to use this version, but better to use the StringRef version."

It seems that this assert isn't consistent with the documentation on the function. I believe one of the two needs to be fixed.

Actually... I see you actually get that right... and my case (having something not officially 'overloaded', but needing the overloaded name) isn't covered by the docs. Sorry 'bout that.