This is an archive of the discontinued LLVM Phabricator instance.

[GlobalOpt] don't change CC of musttail calle(e|r) PR36546
ClosedPublic

Authored by indutny on Feb 27 2018, 10:50 PM.

Details

Summary

When the function has musttail call - its cc is fixed to be equal to the
cc of the musttail callee. In such case (and in the case of the musttail
callee), GlobalOpt should not change the cc to fastcc as it will break
the invariant.

Diff Detail

Event Timeline

indutny created this revision.Feb 27 2018, 10:50 PM

Won't this interfere with tail call optimization? From the language manual, I understood it only to be supported for fastcc. (+ the ghc and HiPE calling conventions, which are already not considered here) If we can change the CC of the whole chain, the invariant would still hold. Would that be a valid alternative?

PS: Please upload full context diffs next time :-)

lib/Transforms/IPO/GlobalOpt.cpp
2137

Wouldn't it be cheaper to check the CC first, do an early return and only check for musttail calls/callees later?

indutny updated this revision to Diff 136358.Feb 28 2018, 12:29 PM
indutny marked an inline comment as done.

Address inline comments.


As far as I can see it, the documentation doesn't say anything about musttail in particular. It says that tail calls require fastcc to be optimized, but even this seems to be rather stale.

Changing CC for the whole chain is totally a valid alternative. Would it make sense to you if I'll add a FIXME comment there?

JDevlieghere accepted this revision.Feb 28 2018, 1:51 PM

Address inline comments.


As far as I can see it, the documentation doesn't say anything about musttail in particular. It says that tail calls require fastcc to be optimized, but even this seems to be rather stale.

Changing CC for the whole chain is totally a valid alternative. Would it make sense to you if I'll add a FIXME comment there?

Alright, that works for me! With the FIXME added this LGTM.

This revision is now accepted and ready to land.Feb 28 2018, 1:51 PM
indutny updated this revision to Diff 136393.Feb 28 2018, 2:02 PM

Added FIXME, as requested.


I'd appreciate if you could land this for me, as I don't have the commit bit yet.

The test case seems to fail for me locally. Can you see if you can reproduce this?

******************** TEST 'LLVM :: Transforms/GlobalOpt/musttail_cc.ll' FAILED ********************
Script:
--
/Users/jonas/llvm/git-svn/build/bin/opt < /Users/jonas/llvm/git-svn/llvm/test/Transforms/GlobalOpt/musttail_cc.ll -globalopt -S | /Users/jonas/llvm/git-svn/build/bin/FileCheck /Users/jonas/llvm/git-svn/llvm/test/Transforms/GlobalOpt/musttail_cc.ll
--
Exit Code: 1

Command Output (stderr):
--
/Users/jonas/llvm/git-svn/llvm/test/Transforms/GlobalOpt/musttail_cc.ll:31:16: error: expected string not found in input
; CHECK-LABEL: define internal i32 @foo2(i32 %a)
               ^
<stdin>:18:35: note: scanning from here
define internal i32 @foo1(i32 %a) unnamed_addr {
                                  ^
<stdin>:19:11: note: possible intended match here
 %ca = musttail call i32 @foo2(i32 %a)
          ^

--
indutny updated this revision to Diff 136400.Feb 28 2018, 2:25 PM

Fix test.


Sorry, looks like I forgot to add internal to the foo2 (or removed it by mistake when rebasing).

This revision was automatically updated to reflect the committed changes.