This is an archive of the discontinued LLVM Phabricator instance.

[IR] Modernize and Cleanup LLVM/IR folder
AbandonedPublic

Authored by gAlfonso-bit on Sep 21 2021, 12:51 PM.

Details

Reviewers
deadalnix
dexonsmith
MaskRay
Group Reviewers
Restricted Project
Summary

No functional changes were made. Instead, warnings were fixed, and optimizations were made

Diff Detail

Event Timeline

gAlfonso-bit created this revision.Sep 21 2021, 12:51 PM
gAlfonso-bit requested review of this revision.Sep 21 2021, 12:51 PM
gAlfonso-bit updated this revision to Diff 374550.EditedSep 23 2021, 7:57 AM

Fixed clang-tidy

ldionne resigned from this revision.Sep 23 2021, 8:51 AM

No functional changes were made.

That doesn't sound correct. You made some constructors explicit, at the very least. Also I'm seeing some function moving from member functions to static functions, and so on.

Regardless, I don't touch this part of the codebase so I'm not sure why I'm marked as a reviewer here.

gAlfonso-bit planned changes to this revision.Sep 23 2021, 9:39 AM
gAlfonso-bit removed a reviewer: ldionne.
gAlfonso-bit abandoned this revision.Nov 12 2021, 9:16 AM

Will split this into many patches

Just a couple of comments inline to keep in mind if/when you start splitting pieces of this out.

llvm/lib/IR/Function.cpp
692–695

Is this an improvement? With this change (and the one to LLVMContext::setGC()), there's potentially allocation traffic with this usage pattern:

std::string GC = /* ... */;
F.setGC(std::move(GC));

where previously there was none.

Another option might be to make this (and the called API) a StringRef, and audit callers to delay any std::string allocation until LLVMContext::setGC().

llvm/lib/IR/IRPrintingPasses.cpp
55–56

When you split this out, I'd suggest using StringRef as the parameter and then Banner(Banner.str()).