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

[Wei Mi via llvm-commits]

wmi added inline comments.

================
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
----------------
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D19884