This is an archive of the discontinued LLVM Phabricator instance.

[Analysis] RemoveTotalMemInst counting in InstCount to avoid reading back other Statistic variables
ClosedPublic

Authored by craig.topper on May 26 2017, 2:55 PM.

Details

Summary

Previously, we counted TotalMemInst by reading certain instruction counters before and after calling visit and then finding the difference. But that wouldn't be thread safe if this same pass was being ran on multiple threads.

This list of "memory instructions" doesn't make sense to me as it includes call/invoke and is missing atomics.

This patch removes the counter all together.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.May 26 2017, 2:55 PM
chandlerc added inline comments.May 26 2017, 7:41 PM
lib/Analysis/InstCount.cpp
44–56 ↗(On Diff #100476)

This seems... a little crazy.

First off, we don't need to increment TotalInsts here. You can just override visitInstruction and teach this to delegate to the base class after incrementing the opcode specific counter. But maybe that doesn't really matter...

But the whole point of InstVisitor is to avoid these long || chains. =[

And what even is a "memory instruction"? Why is a call a memory instruction even if it is calling an LLVM intrinsic? I know you didn't add any of this, but it seems somewhat crazy to bend over backwards to keep it.

How about just deleting the TotalMemInst statistic entirely until someone comes back with a specific use case?

craig.topper retitled this revision from [Analysis] Rewrite TotalMemInst counting in InstCount pass in a way that doesn't require reading back other Statistic variables to [Analysis] RemoveTotalMemInst counting in InstCount to avoid reading back other Statistic variables.
craig.topper edited the summary of this revision. (Show Details)

Just remove the TotalMemInst counter

davide accepted this revision.Jul 16 2017, 9:55 PM
davide added a subscriber: davide.

I'm with Chandler on this one.

This revision is now accepted and ready to land.Jul 16 2017, 9:55 PM
This revision was automatically updated to reflect the committed changes.