Split into three commits:
Reduce code duplication between patchpoint and non-patchpoint lowering. NFC.
Enable invoking the patchpoint intrinsic
Fix an over-negation bug.
Paths
| Differential D5634
Enable invoking the patchpoint intrinsic ClosedPublic Authored by kmod on Oct 6 2014, 5:20 PM.
Details Summary Split into three commits: Reduce code duplication between patchpoint and non-patchpoint lowering. NFC.
Diff Detail Event Timelinekmod updated this object. Comment Actions Remove the refactoring changes from this CR to move to a separate one Just noticed that phabricator doesn't let you view the per-commit diffs, Comment Actions Forgot to include changes to Intrinsics.td specifying that patchpoints can now throw Otherwise the optimizer would just convert the invokes back to calls. Comment Actions Just a few coding style nitpicks inline. I will take a closer look tomorrow and also run it through our test suite to see if there any regressions.
Comment Actions Hi Kevin, the patch doesn’t seem to be against trunk. Could you please post an updated patch against trunk? Cheers, Comment Actions Ok, I rebased onto trunk. One thing I failed to mention is that this patch depends on another that I sent out, D5657, since I wanted to separate some refactoring changes from the actual invoke-supporting change. I switched from a CallInst to an ImmutableCallSite since these functions can now take either a CallInst or an InvokeInst (specifically, visitPatchpoint can now be called from both visitIntrinsicCall and from visitInvoke), and my understanding was that an (Immutable)CallSite is the preferred way to support both. Happy to do it some other way, though. ributzka edited edge metadata. Comment ActionsI still think not threading through ImmutableCallSite would be nicer and reduce unnecessary changes. Otherwise LGTM.
This revision is now accepted and ready to land.Oct 15 2014, 10:49 AM Comment Actions Ok I will update the patch, but I'm not sure how you mean to not thread Comment Actions The patch didn't apply cleanly against trunk. I merged it, but now the test case is failing for me. The test case is very fragile, because it relies on temporary labels. I think you can get rid of most of these checks. Comment Actions Try to make the test a little bit less brittle. I would like to check that the exception data covers Comment Actions Ok I rebased onto r220027, but the test is passing for me. Not that that's really a good sign for the test's robustness, but I'm curious now why it's failing. I noticed that I had used a different triple than all of the other tests used so I fixed that -- I wonder if that makes it pass for you? I'm assuming you're running this on a mac so that an incorrectly-specified triple could cause issues since this assumes ELF. Separately, I also tried to make the test less brittle by getting rid of as many hard-coded label names as I could, but I couldn't get rid of the Ltmp0 one. I wanted to check to make sure the exception data covers the entire patchpoint, since that was one of my worries going into this, though now that I understand the code better I'd be more ok with taking that out.
Revision Contents
Diff 14479 lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
lib/IR/Verifier.cpp
test/CodeGen/X86/patchpoint-invoke.ll
test/Verifier/invoke.ll
|
While you are already at it, could you please clean up the sins I committed and update it to the current coding standard (camelCase for functions and CamelCase for variables)?