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

[Dean Michael Berris via llvm-commits]

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?


https://reviews.llvm.org/D23931