-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[BOLT] Skip functions with unsupported Linux kernel features #86345
Conversation
Do not overwrite functions with alternative and paravirtual instructions until a proper update support is implemented.
✅ With the latest revision this PR passed the Python code formatter. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
Thanks.
if (!BC.shouldEmit(BF)) | ||
break; |
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.
if (!BC.shouldEmit(BF)) | |
break; |
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.
Why? That's intentional early break after we mark the function as non-simple in the loop below.
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.
Oh, that's subtle. Then I'd suggest breaking out immediately after BF.setSimple(false)
to be explicit.
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.
It's breaking, just not to the outer loop. I've refactored using llvm::any_of()
as you suggested and it looks much cleaner now.
for (const MCInst &Inst : BB) { | ||
if (BC.MIB->hasAnnotation(Inst, "ParaSite")) { | ||
BF.setSimple(false); | ||
break; | ||
} | ||
} |
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.
I'm fine with it as-is, but this can help reduce indentation:
for (const MCInst &Inst : BB) { | |
if (BC.MIB->hasAnnotation(Inst, "ParaSite")) { | |
BF.setSimple(false); | |
break; | |
} | |
} | |
bool HasParaSite = llvm::any_of(BB, [&](const MCInst &Inst) { return BC.MIB->hasAnnotation(Inst, "ParaSite"); }); | |
if (HasParaSite) | |
BF.setSimple(false); |
if (!BC.shouldEmit(BF)) | ||
continue; | ||
for (const BinaryBasicBlock &BB : BF) { | ||
if (!BC.shouldEmit(BF)) |
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.
Duplicate
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.
It's not.
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.
Perfect.
Do not overwrite functions with alternative and paravirtual instructions until a proper update support is implemented.