[PATCH] D19884: Fix a bug when hoist spill to a BB with landingpad successor

[Eric Christopher via llvm-commits]

echristo added a reviewer: MatzeB.

================
Comment at: lib/CodeGen/SplitKit.h:124-127
@@ -123,6 +123,6 @@
 
   /// analyze - set CurLI to the specified interval, and analyze how it may be
   /// split.
   void analyze(const LiveInterval *li);
 
   /// didRepairRange() - Returns true if CurLI was invalid and has been repaired
----------------
wmi wrote:
> echristo wrote:
> > I could be missing something but analyze sets the interval and does the analysis which is then cached, but the setParent you've got doesn't do the analysis and so isn't updating aspects of SA right (the computation is happening as part of getLastSplitPointIter, but the clear and analyzeUses isn't happening)? You might not be using it, but it seems odd to leave it in an indeterminate state.
> The cache LastSplitPoint[] in SA is not changed even if CurLI is changed, so we don't need to updated it every time we call setParent (you can see that in SplitAnalysis::clear(), LastSplitPoint is not cleared there. It is also not set by analyzeUses()). 
> 
> SplitAnalysis::clear() is used to clear several caches of other analysises set up in SplitAnalysis::analyzeUses(). Those analysises are unused by hoistspill for now. To make SA a little bit light weighted. I choose not to call analyze(). If other analysises in SplitAnalysis will be used in hoistspill in the future, we can easily change setParent() to analyze() to make them available. 
> 
> I will add a comment before setParent() is called to make it clear.
I see what you mean. Basically getLastSplitPoint could be static if it wasn't for the cache. I worry about surprising people with bugs with setting the LI and then not resetting the rest of the object.

I think this needs a bit more documentation in general. Personally I'm not a huge fan of side-stepping this aspect of the analysis here, but I'm not sure we should pull it out or do something else either. I'm probably going to have to defer to Quentin or Matthias here.


Repository:
  rL LLVM

http://reviews.llvm.org/D19884