[PATCH] D23931: [XRay] ARM 32-bit no-Thumb support in LLVM

Dean Michael Berris via llvm-commits llvm-commits at lists.llvm.org
Sun Sep 4 17:57:51 PDT 2016

dberris added a comment.

In https://reviews.llvm.org/D23931#533606, @rengolin wrote:

> Hi Serge,
> The Nop emission is really simple, and the isXRaySupported() is really simple and accurate. Thanks for addressing all the comments, the code is looking really nice.

+1 -- thanks @rserge!

> Now, two points:


> 1. There are ways to report warnings/errors back to the front-end, but it depends how this is interpreted.


>   Since the instrumentation is inserted by the front end, than this should be a back-end *error*, and front-ends should fail with a decent error message saying "XRay is not supported for target X".


>   If you want just a warning, you can avoid inserting the sleds and the run-time code won't do anything, as you're doing it now. But you then have to warn the users that they won't get what they requested. I strongly suggest to make it an error instead.


>   For error messages, it's best to use "getContext().reportError(Loc, ...)", as this would nicely roll back to the front-end without crashing. But if that doesn't work (it should, really), you can use "report_fatal_error", "llvm_unreachable" or even an "assert()", though these are just last-resort only.


>   About front-end duplicating the checks, it's up to you and @dberris. The error message in Clang and llc should be the same, though, and reportError() does that well.

I'm happy with an error using the usual error reporting mechanisms here.

> 2. Tests.


>   The current test is good, it checks the right number of NOPs and the overall structure. Excellent.


>   Now we need "negative tests", ie. those that *have* to fail. For that, you add a RUN line that starts with "not llc ..." and CHECK for the error messages. There are plenty of examples in there already.


>   Since you're restricting x86_64, you should have one for i386. Since you're restricting ARMv6/Unix, you should have one for ARMv5, and one for ARM Windows.

I agree with this. FWIW, I'm happy with getting this in and getting it tested, then locking it down with more negative tests once it's upstream.

Thanks Renato!

Comment at: lib/Target/ARM/ARMAsmPrinter.h:102-106
@@ +101,7 @@
+  // XRay-specific lowering for ARM.
+  void LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr &MI);
+  void LowerPATCHABLE_FUNCTION_EXIT(const MachineInstr &MI);
+  // Helper function that emits the XRay sleds we've collected for a particular
+  // function.
+  void EmitXRayTable();
Do you already want to support tail call optimisation sleds now? Or did you plan to do something about that later?


More information about the llvm-commits mailing list