Page MenuHomePhabricator

[llvm][NFC][CallSite] Remove CallSite from DeadArgumentElimination
ClosedPublic

Authored by mtrofin on Apr 20 2020, 10:30 PM.

Details

Summary

Also capitalized some induction variables, to match coding style.

Diff Detail

Event Timeline

mtrofin created this revision.Apr 20 2020, 10:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2020, 10:30 PM
craig.topper added inline comments.Apr 20 2020, 10:59 PM
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
565–566

I don't understand why this block exists. We already returned if it wasn't a callsite.

869

This should be a cast I think.

mtrofin updated this revision to Diff 259020.Apr 21 2020, 9:02 AM
mtrofin marked 2 inline comments as done.

feedback

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
565–566

Nice catch! Indeed, 557 took care of it.

mtrofin marked an inline comment as done.Apr 21 2020, 9:02 AM
This revision is now accepted and ready to land.Apr 21 2020, 10:24 AM
This revision was automatically updated to reflect the committed changes.
foad added a subscriber: foad.Apr 21 2020, 11:22 AM
foad added inline comments.
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
444

Needs to be CB->getArgOperand. This is making my DEBUG build fail.

mtrofin marked 2 inline comments as done.Apr 21 2020, 11:24 AM
mtrofin added inline comments.
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
444

Noted, doing a rebuild locally to validate there's no other failures and sending a fix asap. Sorry about this.

lebedev.ri marked an inline comment as done.Apr 21 2020, 11:30 AM
lebedev.ri added a subscriber: lebedev.ri.
lebedev.ri added inline comments.
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
444

(It might be a really good idea to do all local development with LLVM_ENABLE_ASSERTIONS enabled..)

MaskRay added inline comments.
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
444

Just saw the discussions here. I fixed it in cca545ce462b61bdd4bd1b8d81d9eb921e13aebc

For a CMAKE_BUILD_TYPE=Release build, -DLLVM_ENABLE_ASSERTIONS defaults to OFF. It may be good setting it to ON.

mtrofin marked 3 inline comments as done.Apr 21 2020, 12:22 PM
mtrofin added inline comments.
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
444

+1 - good suggestion, you mean locally, I assume.