[BOLT][RISCV] Carry-over annotations when fixing calls #66763
Conversation
`FixRISCVCallsPass` changes all different forms of calls to `PseudoCALL` instructions. However, the original call's annotations were lost in the process. This patch fixes this by moving all annotations from the old to the new call. `MCPlusBuilder::moveAnnotations` had to be made public for this.
| @@ -39,8 +41,19 @@ void FixRISCVCallsPass::runOnFunction(BinaryFunction &BF) { | |||
| auto *Target = MIB->getTargetSymbol(*II); | |||
| assert(Target && "Cannot find call target"); | |||
|
|
|||
| auto OldCall = *NextII; | |||
There was a problem hiding this comment.
We've got a policy to not using auto if it is not necessary, please minimise their usage
There was a problem hiding this comment.
Interesting, up to now my BOLT contributions have been using auto heavily. Is this policy written down somewhere or could you explain when auto is considered ok to use?
There was a problem hiding this comment.
Let the Meta team members to clarify, they even has patches for auto expansion and asked me couple years ago. Maybe the policy changed. At least the LLVM coding standard says:
Don’t “almost always” use auto, but do use auto with initializers like cast(...) or other places where the type is already obvious from the context.
E.g. here it is obviously MCInst or MCSymbol above & etc, so it seems to be fine from this stand point. I don't really insist, but as for me I don't really like autos too except for complex types with smth like returning std::vector with std::pair elements.. Let Meta team members say their opinion, maybe I shall use it more frequently too :)
There was a problem hiding this comment.
Yes, we should stick to LLVM policy of only using auto in small number of cases (the type is obvious (casts), the type doesn't matter (lambdas), template type). Making type explicit helps with reviewing where we have limited context window by default.
|
Looks good but please expand the autos before merging. |
FixRISCVCallsPasschanges all different forms of calls toPseudoCALLinstructions. However, the original call's annotations were lost in the process.This patch fixes this by moving all annotations from the old to the new call.
MCPlusBuilder::moveAnnotationshad to be made public for this.