This is an archive of the discontinued LLVM Phabricator instance.

Add lowering support for llvm.experimental.deoptimize
ClosedPublic

Authored by sanjoy on Mar 23 2016, 11:22 PM.

Details

Summary

Only adds support for "naked" calls to llvm.experimental.deoptimize.
Support for round-tripping through RewriteStatepointsForGC will come
as a separate patch (should be simpler than this one).

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 51515.Mar 23 2016, 11:22 PM
sanjoy retitled this revision from to Add lowering support for llvm.experimental.deoptimize.
sanjoy updated this object.
sanjoy added a reviewer: reames.
sanjoy added a subscriber: llvm-commits.
reames accepted this revision.Mar 24 2016, 11:39 AM
reames edited edge metadata.

LGTM w/comments addressed.

docs/LangRef.rst
12174 ↗(On Diff #51515)

"is available" to "is defined."

12175 ↗(On Diff #51515)

non-varargs is a bit vague here. "Lowered according to the calling convention specified for the deoptimize call as if they were formal arguments of the specified types not vararg." Wording could use improved, but that gets the spirit.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp
925 ↗(On Diff #51515)

It looks like you're only handling calls here. Using CallSite is fine, but add an assertion.

940 ↗(On Diff #51515)

Not sure the exact variable adds anything here.

949 ↗(On Diff #51515)

You might add a comment to make it clear GC args intentionally remain empty.

test/CodeGen/X86/deopt-intrinsic.ll
2 ↗(On Diff #51515)

Doesn't -debug-only require an asserts build?

12 ↗(On Diff #51515)

Might be better to just check for each expected instruction.

13 ↗(On Diff #51515)

Add a check-next for the return. The fact there's nothing in between is worth checking.

21 ↗(On Diff #51515)

Is this how we materialize a floating point constant? That seems like it could be improved in general.

26 ↗(On Diff #51515)

Can you add a test with a different calling convention? There's nothing here that ensures we're using the right one for the lowering as opposed to merely the default.

This revision is now accepted and ready to land.Mar 24 2016, 11:39 AM
sanjoy added inline comments.Mar 24 2016, 11:55 AM
lib/CodeGen/SelectionDAG/StatepointLowering.cpp
940 ↗(On Diff #51515)

Without it I get linker errors around StatepointDirectives::DeoptBundleStatepointID. I didn't spend time figuring out the root cause (I suspect it has to do with the move constructor in getValueOr), and this looks like an acceptable workaround.

This revision was automatically updated to reflect the committed changes.
sanjoy marked 6 inline comments as done.
sanjoy added inline comments.Mar 24 2016, 1:29 PM
lib/CodeGen/SelectionDAG/StatepointLowering.cpp
925 ↗(On Diff #51515)

In retrospect, this can be easily combined with LowerCallSiteWithDeoptBundle. I'll do that in a separate change if feasible.

jlebar added a subscriber: jlebar.Mar 31 2016, 6:32 PM

Hi, I think this may be broken.

In particular, I tried to land rL265092, which turns on an assertion, but had to back it out because it asserts on the test added here. (The assertion is currently only run with -debug, which is doubly bad, because now your code can break for unrelated reasons while you're trying to debug it!)

Would you mind having a look? Bonus points if you reland rL265092 after fixing the issue.

Justin Lebar wrote:
> Hi, I think this may be broken.
>
> In particular, I tried to land http://reviews.llvm.org/rL265092,
> which turns on an assertion, but had to back it out because it asserts
> on the test added here. (The assertion is currently only run with
> -debug, which is doubly bad, because now your code can break for
> unrelated reasons while you're trying to debug it!)

Looks like webkit_jscc does not work with anything but an i64 return
type. The following crashes llc, for instance:

target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-apple-macosx10.11.0"

declare webkit_jscc i32 @foo()

define i32 @caller() {
entry:

%v = call webkit_jscc i32 @foo()
ret i32 %v

}

CC'ing Andy and Juergen, since they might know. For now I'll switch
the test to return i64 and turn on the assertion.

> Would you mind having a look? Bonus points if you reland
> http://reviews.llvm.org/rL265092 after fixing the issue.

  • Sanjoy

Hi,

I've re-enabled the assertion in rL265099, and filed PR27172 for the
webjit_jscc issue.

  • Sanjoy

Sanjoy Das wrote:

sanjoy added a comment.

Justin Lebar wrote:

>  Hi, I think this may be broken.
>
>  In particular, I tried to land http://reviews.llvm.org/rL265092,
>  which turns on an assertion, but had to back it out because it asserts
>  on the test added here.  (The assertion is currently only run with
>  -debug, which is doubly bad, because now your code can break for
>  unrelated reasons while you're trying to debug it!)

Looks like webkit_jscc does not work with anything but an i64 return
type. The following crashes llc, for instance:

target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-apple-macosx10.11.0"

declare webkit_jscc i32 @foo()

define i32 @caller() {
entry:

%v = call webkit_jscc i32 @foo()
ret i32 %v

}

CC'ing Andy and Juergen, since they might know. For now I'll switch
the test to return i64 and turn on the assertion.

Would you mind having a look? Bonus points if you reland

>  http://reviews.llvm.org/rL265092 after fixing the issue.
  • Sanjoy

Repository:

rL LLVM

http://reviews.llvm.org/D18429

atrick added a subscriber: atrick.Apr 1 2016, 9:15 AM

Looks like webkit_jscc does not work with anything but an i64 return
type. The following crashes llc, for instance:

CC'ing Andy and Juergen, since they might know. For now I'll switch
the test to return i64 and turn on the assertion.

That isn't too surprising. Although I thought it worked with f64.

We don't need to keep the webkit_jscc convention. I think its only purpose is to test codegen for patchpoints. Juergen?