-
Notifications
You must be signed in to change notification settings - Fork 10.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP getting master-next up and running with clang::ModuleMacro #10519
WIP getting master-next up and running with clang::ModuleMacro #10519
Conversation
cc @compnerd, @bob-wilson |
I haven't been able to test this because of various other problems on the master-next branch. |
I'm not a SIL author, so it's likely that this could be improved, but I think this is closest to the old LLVM behavior.
The tests still need to be updated.
b02d6a8
to
dc16e16
Compare
cc @gottesmm |
@jrose-apple Thanks Jordan! |
Also cc @lhames |
btw, @jrose-apple cross repo testing works with master-next if you want to try the two PRs together. |
Still plenty of local failures for now. :-( |
Untested and still missing a few pieces.
dc16e16
to
c3d6be6
Compare
Okay, this isn't right yet, but it's back in a shape where we can make progress. Let's see where we are. apple/swift-clang#93 |
Build failed |
Build failed |
@jrose-apple Just took a quick look at the failure on OS X (for some reason, the ci display is saying that builds are still running, but I think the display is wrong). The error is:
That suggests we need to call in @adrian-prantl. |
@@ -355,3 +355,7 @@ bool SILBasicBlock::isTrampoline() const { | |||
return false; | |||
return begin() == Branch->getIterator(); | |||
} | |||
|
|||
bool SILBasicBlock::isLegalToHoistInto() const { | |||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gottesmm, is this the right implementation for now? If so I'll extract it out into a separate PR so we can land it and be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I talked with Arnold and I don't think we use this. If you do extract this, can you change the return to false so we are conservative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jordan pointed out via IM that this is used in other parts of loop info that we may use and that the old behavior was to always hoist. So given that, I am fine just taking this if it gets the build going. We can fix the test failures later. But at least we know things build.
Jordan says that he is unavailable to merge this. I am going to merge this for him so we can get the build going. |
For pre-review purposes, mostly. The interesting commit is the one about ModuleMacro, though really we should be able to apply that to regular-master when it's ready.