[PATCH] D100818: [lld-macho] Implement branch-range-extension thunks

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

I think you haven't uploaded the latest changes



================
Comment at: lld/MachO/Writer.cpp:545
     }
+  } else if (sym != config->entry) {
+    llvm_unreachable("invalid branch target symbol type");
----------------
gkm wrote:
> int3 wrote:
> > why is this `config->entry` check necessary?
> Because of this ...
> ```
> template <class LP> void Writer::run() {
>   prepareBranchTarget(config->entry);
>   . . .
> ```
> ... and this ...
> ```
> bool macho::link(ArrayRef<const char *> argsArr, bool canExitEarly,
>                  raw_ostream &stdoutOS, raw_ostream &stderrOS) {
>   . . .
>   config->entry = symtab->addUndefined(args.getLastArgValue(OPT_e, "_main"),
>                                        /*file=*/nullptr,
>                                        /*isWeakRef=*/false);
>   . . .
> ```
ah. I think it'd make more sense not to call `prepareBranchTarget` with an undefined symbol in the first place...


================
Comment at: lld/MachO/Writer.cpp:564
     prepareBranchTarget(sym);
+    r.isCallSite = true;
+    isec->callSiteCount++;
----------------
gkm wrote:
> int3 wrote:
> > can't we just check for `relocAttrs.hasAttr(RelocAttrBits::BRANCH)` instead of adding a new property on Reloc?
> After seeing your abandoned diff about the performance drag of checking `relocAttrs`, I chose to save the work of doing so again. Perhaps it is a micro-efficiency that I should avoid?
yes please. If you've noticed the theme of my other comments... let's write the simplest code possible first, and optimize it if profile data indicates it's worthwhile :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100818/new/

https://reviews.llvm.org/D100818