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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:169
@@ -168,3 +168,3 @@
 
-  instrprof_error Result = instrprof_error::success;
+  SoftInstrProfErrors SIPE;
   if (NewFunc) {
----------------
vsk wrote:
> 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.
I think it is reasonable to make the InstrProfValueSiteRecord's interfaces to take SPIE reference as new parameter. Make SPIE member of InstrProfRecord and make its interface unchanged.

 Would that work?


http://reviews.llvm.org/D20082