This is an archive of the discontinued LLVM Phabricator instance.

[llvm][NFC] Refactor uses of CallSite to CallBase - call promotion
ClosedPublic

Authored by mtrofin on Apr 10 2020, 8:38 PM.

Details

Summary

Updated CallPromotionUtils and impacted sites. Parameters that are
expected to be non-null, and return values that are guranteed non-null,
were replaced with CallBase references rather than pointers.

Left FIXME in places where more changes are facilitated by CallBase, but
aren't CallSites: Instruction* parameters or return values, for example,
where the contract that they are actually CallBase values.

Diff Detail

Event Timeline

mtrofin created this revision.Apr 10 2020, 8:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2020, 8:38 PM
dblaikie accepted this revision.Apr 11 2020, 5:25 PM
dblaikie added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUFixFunctionBitcasts.cpp
39

Hmm, might be worth adding an implicit conversion from CallSite to CallBase* to make code like this easier? (alternatively/at least CallSite::getInstruction() should return CallBase*, I think?)

llvm/unittests/Transforms/Utils/CallPromotionUtilsTest.cpp
94–96

Can probably drop the intermediate CB variable now (& just write CI->getCalledFunction() and tryPromoteCall(*CI))

(similarly below)

This revision is now accepted and ready to land.Apr 11 2020, 5:25 PM
mtrofin updated this revision to Diff 256813.Apr 11 2020, 6:51 PM
mtrofin marked 2 inline comments as done.

simplified CallPromotionUtilsTest.cpp

mtrofin marked 2 inline comments as done.Apr 11 2020, 6:53 PM
mtrofin added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUFixFunctionBitcasts.cpp
39

I'd keep it ugly and hard to use, since we want to deprecate CallSite, and encourage more refactoring.

(I'm hoping to actually complete it, I think it's very tractable)

llvm/unittests/Transforms/Utils/CallPromotionUtilsTest.cpp
94–96

Done.

Also, seems like this file would benefit from ASSERT_TRUE(pointer) -> ASSERT_NE(pointer, nullptr) (same for EXPECT_) - or is this not particularly followed?

mtrofin marked an inline comment as done.Apr 11 2020, 7:03 PM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Apr 13 2020, 12:57 PM
llvm/unittests/Transforms/Utils/CallPromotionUtilsTest.cpp
94–96

When writing non-gtest boolean tests, I favor "if (X)" or "if (!X)" but I'm not sure what's most common across LLVM.

For gtests - yeah, I think using the most descriptive test is good from a gtest error message perspective - I don't know of any particular convention there, but I wouldn't object to/would encourage changes to go in that direction (of using the most descriptive tests).