[Lldb-commits] [PATCH] D154930: [lldb][AArch64] Add the tpidr2 TLS register that comes with SME

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py:73
+
+    @skipUnlessArch("aarch64")
+    @skipUnlessPlatform(["linux"])
----------------
DavidSpickett wrote:
> omjavaid wrote:
> > These three tests have a lot of commonalities may be merge them into one testing the whole logic. Doesn't look like we are getting much out of emitting three tests from this fairly basic test class.
> The tradeoff is execution time vs. a HWCAP check in the program file and a bunch of ifs in Python.
> 
> Let me see what I can do, but I'm leaning toward the implementation complexity outweighing the performance gained.
I've looked into this. A major thing to note is that reading tpidr2 (or rather, the system register operands that represent it) on a system without SME will SIGILL. I tried this on a Mountain Jade system.

This means we cannot have the program file set both regardless and only test what we expect to be present. We still need the option to the program and still need to run it again each time we want to check a different register.

(I guess you could have an option for each register, but again we're trading complexity here)

That does mean that the program file is the same between the 3 tests, so you could merge them to only build once. That said I think the time saved rebuilding a small example is not much when put against the pile of `if` you would need to use to merge the 3 tests into one (and therefore have to unpick later).

I could merge `test_tpidr2_no_sme` into `test_tpidr` but again, I'd rather have the clear separation of it being its own test than add an `if not self.isAArch64SME...` in there.

If I'm addressing completely different aspects than you had considered here, let me know and I'll fix those.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154930/new/

https://reviews.llvm.org/D154930