Re: [PATCH 6/7] AArch64 - new pass to add conditional-branch speculation tracking
On 07/23/2018 08:33 AM, Richard Earnshaw (lists) wrote: > [sorry, missed this mail somehow] > > On 11/07/18 22:01, Jeff Law wrote: >> On 07/09/2018 10:38 AM, Richard Earnshaw wrote: >>> This patch is the main part of the speculation tracking code. It adds >>> a new target-specific pass that is run just before the final branch >>> reorg pass (so that it can clean up any new edge insertions we make). >>> The pass is only run with -mtrack-speculation is passed on the command >>> line. >>> >>> One thing that did come to light as part of this was that the stack pointer >>> register was not being permitted in comparision instructions. We rely on >>> that for moving the tracking state between SP and the scratch register at >>> function call boundaries. >> Note that the sp in comparison instructions issue came up with the >> improvements to stack-clash that Tamar, Richard S. and you worked on. >> > > I can certainly lift that part into a separate patch. Your call. It was mostly an observation that the change was clearly needed elsewhere. I'm certainly comfortable letting that hunk go in with whichever kit is approved first :-) > >> >>> >>> * config/aarch64/aarch64-speculation.cc: New file. >>> * config/aarch64/aarch64-passes.def (pass_track_speculation): Add before >>> pass_reorder_blocks. >>> * config/aarch64/aarch64-protos.h (make_pass_track_speculation): Add >>> prototype. >>> * config/aarch64/aarch64.c (aarch64_conditional_register_usage): Fix >>> X14 and X15 when tracking speculation. >>> * config/aarch64/aarch64.md (register name constants): Add >>> SPECULATION_TRACKER_REGNUM and SPECULATION_SCRATCH_REGNUM. >>> (unspec): Add UNSPEC_SPECULATION_TRACKER. >>> (speculation_barrier): New insn attribute. >>> (cmp): Allow SP in comparisons. >>> (speculation_tracker): New insn. >>> (speculation_barrier): Add speculation_barrier attribute. >>> * config/aarch64/t-aarch64: Add make rule for aarch64-speculation.o. >>> * config.gcc (aarch64*-*-*): Add aarch64-speculation.o to extra_objs. >>> * doc/invoke.texi (AArch64 Options): Document -mtrack-speculation. >>> --- >>> gcc/config.gcc| 2 +- >>> gcc/config/aarch64/aarch64-passes.def | 1 + >>> gcc/config/aarch64/aarch64-protos.h | 3 +- >>> gcc/config/aarch64/aarch64-speculation.cc | 494 >>> ++ >>> gcc/config/aarch64/aarch64.c | 13 + >>> gcc/config/aarch64/aarch64.md | 30 +- >>> gcc/config/aarch64/t-aarch64 | 10 + >>> gcc/doc/invoke.texi | 10 +- >>> 8 files changed, 558 insertions(+), 5 deletions(-) >>> create mode 100644 gcc/config/aarch64/aarch64-speculation.cc >> Given the consensus forming about using these kind of masking >> instructions being the preferred way to mitigate (as opposed to lfence >> barriers and the like) I have to ask your opinions about making the bulk >> of this a general pass rather than one specific to the aarch backend. >> I'd hate to end up duplicating all this stuff across multiple architectures. >> >> I think it all looks pretty reasonable though. >> >> jeff >> > > > It would be nice to make this more generic, but I'm not sure how easy > that would be. Some of the analysis is surely the same, but deployment > of the mitigation itself is perhaps more complex. At this point in > time, I think I'd prefer to go with the target-specific implementation > and then look to generalize it as a follow-up. There may be some more > optimizations to add later as well. ACK. I suspect it's mostly the analysis side that we'll want to share. I don't mind giving you the advantage of going first and letting it live in the aarch64 backend. Second implementation can extract the analysis bits :-) So IMHO, this can go forward whenever you want to push it. Jeff
Re: [PATCH 6/7] AArch64 - new pass to add conditional-branch speculation tracking
[sorry, missed this mail somehow] On 11/07/18 22:01, Jeff Law wrote: > On 07/09/2018 10:38 AM, Richard Earnshaw wrote: >> This patch is the main part of the speculation tracking code. It adds >> a new target-specific pass that is run just before the final branch >> reorg pass (so that it can clean up any new edge insertions we make). >> The pass is only run with -mtrack-speculation is passed on the command >> line. >> >> One thing that did come to light as part of this was that the stack pointer >> register was not being permitted in comparision instructions. We rely on >> that for moving the tracking state between SP and the scratch register at >> function call boundaries. > Note that the sp in comparison instructions issue came up with the > improvements to stack-clash that Tamar, Richard S. and you worked on. > I can certainly lift that part into a separate patch. > >> >> * config/aarch64/aarch64-speculation.cc: New file. >> * config/aarch64/aarch64-passes.def (pass_track_speculation): Add before >> pass_reorder_blocks. >> * config/aarch64/aarch64-protos.h (make_pass_track_speculation): Add >> prototype. >> * config/aarch64/aarch64.c (aarch64_conditional_register_usage): Fix >> X14 and X15 when tracking speculation. >> * config/aarch64/aarch64.md (register name constants): Add >> SPECULATION_TRACKER_REGNUM and SPECULATION_SCRATCH_REGNUM. >> (unspec): Add UNSPEC_SPECULATION_TRACKER. >> (speculation_barrier): New insn attribute. >> (cmp): Allow SP in comparisons. >> (speculation_tracker): New insn. >> (speculation_barrier): Add speculation_barrier attribute. >> * config/aarch64/t-aarch64: Add make rule for aarch64-speculation.o. >> * config.gcc (aarch64*-*-*): Add aarch64-speculation.o to extra_objs. >> * doc/invoke.texi (AArch64 Options): Document -mtrack-speculation. >> --- >> gcc/config.gcc| 2 +- >> gcc/config/aarch64/aarch64-passes.def | 1 + >> gcc/config/aarch64/aarch64-protos.h | 3 +- >> gcc/config/aarch64/aarch64-speculation.cc | 494 >> ++ >> gcc/config/aarch64/aarch64.c | 13 + >> gcc/config/aarch64/aarch64.md | 30 +- >> gcc/config/aarch64/t-aarch64 | 10 + >> gcc/doc/invoke.texi | 10 +- >> 8 files changed, 558 insertions(+), 5 deletions(-) >> create mode 100644 gcc/config/aarch64/aarch64-speculation.cc > Given the consensus forming about using these kind of masking > instructions being the preferred way to mitigate (as opposed to lfence > barriers and the like) I have to ask your opinions about making the bulk > of this a general pass rather than one specific to the aarch backend. > I'd hate to end up duplicating all this stuff across multiple architectures. > > I think it all looks pretty reasonable though. > > jeff > It would be nice to make this more generic, but I'm not sure how easy that would be. Some of the analysis is surely the same, but deployment of the mitigation itself is perhaps more complex. At this point in time, I think I'd prefer to go with the target-specific implementation and then look to generalize it as a follow-up. There may be some more optimizations to add later as well. R.
Re: [PATCH 6/7] AArch64 - new pass to add conditional-branch speculation tracking
On 07/09/2018 10:38 AM, Richard Earnshaw wrote: > This patch is the main part of the speculation tracking code. It adds > a new target-specific pass that is run just before the final branch > reorg pass (so that it can clean up any new edge insertions we make). > The pass is only run with -mtrack-speculation is passed on the command > line. > > One thing that did come to light as part of this was that the stack pointer > register was not being permitted in comparision instructions. We rely on > that for moving the tracking state between SP and the scratch register at > function call boundaries. Note that the sp in comparison instructions issue came up with the improvements to stack-clash that Tamar, Richard S. and you worked on. > > * config/aarch64/aarch64-speculation.cc: New file. > * config/aarch64/aarch64-passes.def (pass_track_speculation): Add before > pass_reorder_blocks. > * config/aarch64/aarch64-protos.h (make_pass_track_speculation): Add > prototype. > * config/aarch64/aarch64.c (aarch64_conditional_register_usage): Fix > X14 and X15 when tracking speculation. > * config/aarch64/aarch64.md (register name constants): Add > SPECULATION_TRACKER_REGNUM and SPECULATION_SCRATCH_REGNUM. > (unspec): Add UNSPEC_SPECULATION_TRACKER. > (speculation_barrier): New insn attribute. > (cmp): Allow SP in comparisons. > (speculation_tracker): New insn. > (speculation_barrier): Add speculation_barrier attribute. > * config/aarch64/t-aarch64: Add make rule for aarch64-speculation.o. > * config.gcc (aarch64*-*-*): Add aarch64-speculation.o to extra_objs. > * doc/invoke.texi (AArch64 Options): Document -mtrack-speculation. > --- > gcc/config.gcc| 2 +- > gcc/config/aarch64/aarch64-passes.def | 1 + > gcc/config/aarch64/aarch64-protos.h | 3 +- > gcc/config/aarch64/aarch64-speculation.cc | 494 > ++ > gcc/config/aarch64/aarch64.c | 13 + > gcc/config/aarch64/aarch64.md | 30 +- > gcc/config/aarch64/t-aarch64 | 10 + > gcc/doc/invoke.texi | 10 +- > 8 files changed, 558 insertions(+), 5 deletions(-) > create mode 100644 gcc/config/aarch64/aarch64-speculation.cc Given the consensus forming about using these kind of masking instructions being the preferred way to mitigate (as opposed to lfence barriers and the like) I have to ask your opinions about making the bulk of this a general pass rather than one specific to the aarch backend. I'd hate to end up duplicating all this stuff across multiple architectures. I think it all looks pretty reasonable though. jeff
