fhahn wrote: > I haven't looked closely to the patch, but I share @MatzeB's concerns here. > > Essentially this patch is reverting https://reviews.llvm.org/D36160, which > was fixing a modeling issue with LR on ARM to begin with!
Thanks for sharing the additional context and where `IsRestored` is actually set. Taking a look at the original patch, it seems like it doesn't properly account for the fact that there could be multiple return blocks which may not be using `POP` to restore LR to PC. This could be due to shrink-wrapping + tail-calls in some exit blocks (like in `outlined-fn-may-clobber-lr-in-caller.ll`) or possibly some return instructions not using POP. IIUC D36160 tries to track LR liveness more accurately and doesn't fix a miscompile, but potentially introduced an mis-compile due to underestimating liveness of LR. I don't think the current interface allows to properly check if all exit blocks are covered by POP insts in `restoreCalleeSavedRegisters`, as it works on single return blocks only. Without changing the API, we could check if LR is marked as not restored, and if it is check if there are multiple return blocks, as sketched in https://gist.github.com/fhahn/67937125b64440a8a414909c4a1b7973. This could be further refined to check if POP could be used for all returns (not sure if it is worth it given the small impact on the tests) or the API could be changed to pass all return blocks to avoid re-scanning for returns on each call (not sure if we should extend the general API even more for this workaround though). WDYT https://github.com/llvm/llvm-project/pull/73553 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
