<div dir="ltr"><div>Hi Sergio,</div><div><br></div><div>Thank you for your patch. You'll see that I already reviewed it before seeing this email and have just added a couple of others to the patch as reviewers who I know work in this area.</div><div><br></div><div>Functional changes that improve compatibility with the GNU equivalent binutil are always welcome. The general opinion is that we should aim for compatibility except for extreme cases where what GNU does is clearly a bug or it doesn't matter (e.g. in llvm-objcopy, our output is sometimes slightly different, but we aim for the same semantic behaviour).<br></div><div><br></div><div>I'm not really familiar with how InstPrinter is used outside llvm-objdump. You should make sure to run the `check-all` build target to ensure there is no other testing that relies on the behaviour you have changed.</div><div><br></div><div>I don't think we want to be in a place where the behaviour is different across different targets. Consequently, I'd make sure you check and change the behaviour for the other targets too, and add testing for those changes.</div><div><br></div><div>Let us know if you have any more questions!</div><div><br></div><div>James<br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 13 Dec 2019 at 17:22, Sergio Pérez via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Hello All,</div><div><br></div>While using llvm-objdump I noticed that for the pc-rel branching instructions (jmp/br/call) it prints the immediate value encoded in the instruction, as opposed to the target address like binutils does.<br>I've created a patch (<a href="https://reviews.llvm.org/D71453" target="_blank">https://reviews.llvm.org/D71453</a>) that tries to match the output of those instructions with binutils, my first attempt to contribute. Since I modify the target specific 'InstPrinter's I only did it (initially) for the targets that have llvm-objdump tests, and also I assume InstPrinter might be used in more components other than objdump, therefore I would like to kindly ask for you feedback.<br><div><br></div><div>E.g. </div><div><br></div><div>Binutils:</div><div>4005af:     0f 8f 35 00 00 00     jg     4005ea <main+0x8a><br></div><div><br></div><div>llvm-objdump (currently):</div><div>4005af: 0f 8f 35 00 00 00              jg      0x35 <main+0x8a><br></div><div><br></div><div>Best regards,</div><div>Sergio</div></div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>