[PATCH] D38730: X86: Fix X86CallFrameOptimization to search for the COPY StackPointer

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


================
Comment at: llvm/trunk/lib/Target/X86/X86CallFrameOptimization.cpp:393
 
-  while ((Classification = classifyInstruction(MBB, I, RegInfo, UsedRegs)) !=
-         Exit) {
-    if (Classification == Skip) {
-      ++I;
+  for (InstClassification Classification = Skip; Classification != Exit; ++I) {
+    // If this is the COPY of the stack pointer, it's ok to ignore.
----------------
DavidKreitzer wrote:
> I think there is a potential problem with this new loop structure. If the loop terminates by reaching MBB.end(), the code will attempt to advance the iterator beyond MBB.end().  Having the loop terminate by reaching MBB.end() is probably not an expected situation, but it seems like we should fix this regardless (unless I'm misunderstanding something).
> 
In general, your concern is valid as it is possible that the frame-setup,call,frame-destroy instructions be located in exclusive BB's. FWIW, MachineVerifier::verifyStackFrame checks for a weaker requirement.
However, in this pass,  X86CallFrameOptimization::isLegal requires that the entire call chain be located in a single BB. See the comment starting with "You would expect straight-line code...".
I came across this dilemma when adding the loop in line 375. That code assumes we meet the call instruction before end of the BB, which is only correct under the assumption of all-in-same-BB
I made a note to do some clean-up on this pass of removing overly-conservative checks of reaching end of BB. And of course, we should consider improving this pass to handle the multi-BB call chains.


Repository:
  rL LLVM

https://reviews.llvm.org/D38730