Re: [PATCH 6/7] AArch64 - new pass to add conditional-branch speculation tracking

[Jeff Law <law@redhat.com>]

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<mode>): 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