[PATCH] D20082: [ProfileData] Use SoftInstrProfErrors to count soft errors

[Vedant Kumar via llvm-commits]

vsk added inline comments.

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:169
@@ -168,3 +168,3 @@
 
-  instrprof_error Result = instrprof_error::success;
+  SoftInstrProfErrors SIPE;
   if (NewFunc) {
----------------
davidxl wrote:
> vsk wrote:
> > davidxl wrote:
> > > Do we really need to pass down the instance reference through all the interfaces? I suppose there should be one singleton instance of error, so some global error update APIs will do?
> > Good point. Wdyt of:
> > 
> > 1. Adding a SoftInstrProfErrors member to InstrProfRecord, along with a getError() method (eventually takeError()).
> > 2. Passing a SoftInstrProfErrors& to the InstrProfValueSiteRecord constructor.
> > 
> > That should un-clutter the interfaces.
> > 
> > A global SoftInstrProfErrors instance could see contention, and I'd like to avoid that. It would also make it awkward to enforce soft error handling once we move to Error/Expected: who would be responsible for handling errors which come out of a given record?
> sounds good -- the error is per-instrRecord anyway.
I wasn't able to add a SoftInstrProfErrors member to InstrProfRecord.

Maintaining a reference to a SIPE instance in InstrProfValueSiteRecord seems to be brittle. If an InstrProfRecord is moved/copied, each entry in IndirectCallSites has to be updated. After adding move/copy constructors to handle this, I found other cases where `&InstrProfRecord.SIPE != &IndirectCallSites[I].SIPE` but didn't finish investigating the issue. Given that there's extra bookkeeping overhead with this approach (space for the references + time to correct the references per move/copy), I'd prefer keeping the initial version of this patch.

I considered adding a SoftInstrProfErrors _reference_ to InstrProfRecords, but this would cause a lot of churn in the unit tests. I'm open to other approaches.


http://reviews.llvm.org/D20082