omjavaid added a comment.
We are using SVE read/write in this test to make sure we do not overlap
register offsets while calculating offsets for the dynamic registers. Further
dynamic register sets should also be readable.
================
Comment at:
lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:35
+
+ p_value_bytes = ['0xff', '0x55', '0x11', '0x01', '0x00']
+ for i in range(16):
----------------
DavidSpickett wrote:
> Can you explain the logic for the values here?
P reg sets predicate lanes. P0 will have all lanes set while P5 will have no
lanes set. These are just random values testing read/write.
================
Comment at:
lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:39
+ ' '.join(p_value_bytes[i % 5] for _ in range(p_reg_size)) + '}'
+ self.expect("register read " + 'p%i' % (i), substrs=[p_regs_value])
+
----------------
DavidSpickett wrote:
> You don't need the brackets around i for `% (i)`.
Ack.
================
Comment at:
lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:49
+
+ for i in range(32):
+ self.runCmd('register write ' + 'z%i' %
----------------
DavidSpickett wrote:
> Is it slightly more strict if we have one loop that reads then writes?
> ```
> for i in range(32):
> write
> read
> ```
>
> Since we write the same value, if you write them all then read them all you
> aren't totally sure that each one is lined up right. Then again I'm sure
> you've tested sve register writes elsewhere.
>
> Anyway, a single loop for each would be a bit neater.
I think you are right. If we do a read after write in a single loop we ll be
doing 64 ptrace calls on lldb-server side. More the better.
================
Comment at:
lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:111
+
+ if not self.isAArch64SVE() and sve_regset_available:
+ self.fail(
----------------
DavidSpickett wrote:
> If you move all this code from after the for, into the for loop, you could
> skip having the bools per register set.
I was actually trying to see whether what cpuinfo reports and what is reported
by prctl is same or not.
I am going to revisit this piece and adjust.
================
Comment at:
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:14
+ asm volatile("setffr\n\t");
+ asm volatile("ptrue p0.b\n\t");
+ asm volatile("ptrue p1.h\n\t");
----------------
DavidSpickett wrote:
> (I'm not familiar with SVE assembly but anyway..)
>
> Is the .b/.h/.s etc. pattern specific or does it not matter?
Replied in comment below.
================
Comment at:
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:31-32
+
+ asm volatile("cpy z0.b, p0/z, #1\n\t");
+ asm volatile("cpy z1.b, p5/z, #2\n\t");
+ asm volatile("cpy z2.b, p10/z, #3\n\t");
----------------
DavidSpickett wrote:
> Same here, is the p0/p5/p10/p15 pattern affecting values used in the test or
> just for fun? (which isn't a bad thing)
I copied this from SVE test case that I wrote last year.
P is a predicate register and ptrue/pfalse instruction is used to set a
predicate lane with a pattern. pattern is decide based on size specifier, b, h,
s and d.
if size specified is b all lanes will be set to 1. which is needed to set al
bytes in a Z registers to the specified value.
We are setting p0, p5, p10 and p15 to enable all lanes other p registers are
not enabling all lanes thats why they were not used as predicate for setting Z
registers in following lines which set a constant value to Z register byte size
elements.
================
Comment at:
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:68
+ unsigned int mte_is_enabled = 0;
+ unsigned int pauth_is_enabled = 0;
+
----------------
DavidSpickett wrote:
> This appears to be defined but not set.
Ack. I ll adjust these as per your suggestion below.
================
Comment at:
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:73
+
+ if (hwcap & HWCAP_SVE) // check if SVE is present
+ set_sve_registers();
----------------
DavidSpickett wrote:
> Should there be a `#ifndef HWCAP_SVE` above too?
Most distros have now backported ptrace.h to include HWCAP_SVE but other two
are still in the process.
================
Comment at:
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:82
+ if (hwcap2 & HWCAP2_MTE)
+ mte_is_enabled = 1;
+
----------------
DavidSpickett wrote:
> Would be neat to do these vars like:
> ```
> unsigned int mte_is_enabled = hwcap2 & HWCAP2_MTE;
> ```
Cool!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96463/new/
https://reviews.llvm.org/D96463
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
