Re: [PATCH, BINUTILS, AARCH64, 5/8] Add Tag getting instruction in Memory Tagging Extension

[Sudakshina Das <Sudi dot Das at arm dot com>]

Hi Richard

On 30/10/18 10:15, Richard Earnshaw (lists) wrote:
> On 09/10/2018 18:25, Sudakshina Das wrote:
>> Hi
>>
>> This patch is part of the patch series to add support for ARMv8.5-A
>> Memory Tagging Extensions.
>> (https://developer.arm.com/products/architecture/cpu-architecture/a-profile/exploration-tools)
>>
>> Memory Tagging Extension (MTE) is an optional extension to
>> ARMv8.5-A and is enabled using the +memtag command line option.
>>
>> This patch add support to the Tag Getting instruction from
>> MTE:
>> - LDG <Xt>, [<Xn|SP>, #<simm>]
>>
>> where
>> <Xt> : Is the 64-bit destination GPR.
>> <Xn|SP> : Is the 64-bit first source GPR or Stack pointer.
>> <simm> : Is the optional signed immediate offset, a multiple of 16
>> in the range of -4096 and 4080, defaulting to 0.
>>
>> Testing done: Builds and reg tests all pass on aarch64-none-linux-gnu.
>> Added tests.
>>
>> Is this ok for trunk?
>>
>> Thanks
>> Sudi
>>
>> *** opcodes/ChangeLog ***
>>
>> 2018-xx-xx  Sudakshina Das  <sudi.das@arm.com>
>>
>>      * aarch64-tbl.h (QL_LDG): New.
>>      (aarch64_opcode_table): Add ldg.
>>      * aarch64-asm-2.c: Regenarated.
>>      * aarch64-dis-2.c: Regenerated.
>>      * aarch64-opc-2.c: Regenerated.
>>
>> *** gas/ChangeLog ***
>>
>> 2018-xx-xx  Sudakshina Das  <sudi.das@arm.com>
>>
>>      * testsuite/gas/aarch64/armv8_5-a-mte.s: Add tests for ldg.
>>      * testsuite/gas/aarch64/armv8_5-a-mte.d: Likewise.
>>
>>
> 
> Same mte/memtag issue as earlier patches.
> 
> What's the rationale behind this set of test instructions?
> 
> 
> +.*:	d96001d5 	ldg	x21, \[x14\]
> +.*:	d960a1d5 	ldg	x21, \[x14, #160\]
> +.*:	d961e1d5 	ldg	x21, \[x14, #480\]
> +.*:	d962d1d5 	ldg	x21, \[x14, #720\]
> +.*:	d965a1d5 	ldg	x21, \[x14, #1440\]
> +.*:	d96c81d5 	ldg	x21, \[x14, #3200\]
> +.*:	d96f51d5 	ldg	x21, \[x14, #3920\]
> +.*:	d97fb30c 	ldg	x12, \[x24, #-80\]
> +.*:	d97ec30c 	ldg	x12, \[x24, #-320\]
> +.*:	d97dd30c 	ldg	x12, \[x24, #-560\]
> +.*:	d97d330c 	ldg	x12, \[x24, #-720\]
> +.*:	d97a530c 	ldg	x12, \[x24, #-1456\]
> +.*:	d973730c 	ldg	x12, \[x24, #-3216\]
> +.*:	d970330c 	ldg	x12, \[x24, #-4048\]
> +.*:	d96ff3e1 	ldg	x1, \[sp, #4080\]
> +.*:	d97003eb 	ldg	x11, \[sp, #-4096\]
> 
> It doesn't seem particularly scientific to me.
> 
> Generally gas tests need to:
> 
> - test the base encoding of the instruction (try to find (a minimal set
> of) examples where all the operand fields are not set)
> - test each operand independently with a minimal set of tests that
> ensure the bits are encoded in the correct places.  eg if you have a
> contiguous 5-bit field, try to set all the bits in it: if x31/sp, that
> is sufficient.  Fields that have multiple insertion points (ie are split
> across the instruction need a bit more care).
> - a couple of random values, just to cover some other cases - try to
> avoid combinations that look too similar to other tests in the suite.
> 
> - test that invalid operands are rejected (don't permit xzr/sp if the
> instruction should not allow it, reject invalid immediates - don't
> forget invalid cases like not-a-multiple-of as well as too large/small).
> 
> So tests here should probably be something like:
> 
> ldg	x0, [x0]	// base pattern
> ldg	x21, [x0]	// sets top and bottom bits of load register
> ldg	x0, [sp]	// top-and-bottom of address
> ldg	x0, [x0, #-4096] // minimum negative offset
> ldg	x0, [x0, #4080] // maximum positive offset
> ldg	x8, [x10, #720] // random tests...
> 
> ldg	xzr, [x0]	// error, xzr not valid
> ldg	x0, [x0, #3]	// error, invalid offset
> ...
> 
> If you have access to another disassembler, then results should be cross
> checked with that.  Otherwise each test value needs to be manually
> verified for accuracy (otherwise the test doesn't tell us anything).
> 

As answered before on patch 2/8
Thanks you for test case suggestions. I have also updated the tests and
added a test for the illegal tests. The illegal testing has also
allowed me to fish out a couple of bugs.

I have changed the general format of the positive tests to make it
better to read. All macros are moved on top and every instruction has
the following testing pattern:
1) Base test with x0 and #0 depending on the instruction
2) Macro call to add some random tests of different combinations.
3) Special casing some SP register and immediate corner cases depending
on the instruction.

> R.
> 
*** opcodes/ChangeLog ***

2018-xx-xx  Sudakshina Das  <sudi.das@arm.com>

	* aarch64-tbl.h (QL_LDG): New.
	(aarch64_opcode_table): Add ldg.
	* aarch64-asm-2.c: Regenerated.
	* aarch64-dis-2.c: Regenerated.
	* aarch64-opc-2.c: Regenerated.

*** gas/ChangeLog ***

2018-xx-xx  Sudakshina Das  <sudi.das@arm.com>

	* testsuite/gas/aarch64/armv8_5-a-memtag.s: Add tests for ldg.
	* testsuite/gas/aarch64/armv8_5-a-memtag.d: Likewise.
	* testsuite/gas/aarch64/illegal-memtag.s: Likewise.
	* testsuite/gas/aarch64/illegal-memtag.l: Likewise.

Thanks
Sudi

Attachment: patch5.tar.gz
Description: patch5.tar.gz