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).
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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. |
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. |
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
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?