[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.
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