And rename pr_data to PrData. This is a simple variable that does
not have strong needs to stick with the name used in System V
Application Binary Interface Linux Extensions.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
You do not need to do cosmetic changes in the same patch where you do functional changes generally.
i.e. renaming of the pr_data -> PrData is LGTM and it can be committed as NFC change that does not need a review.
Regarding the rest of the patch, seems it is not fully covered with a test case? I see no output for AVX512BW for example?
There is no output for AVX512BW, as I'm not sure enumerating every bit would be useful. Every bit can be tested individually, but that is tedious and makes the test hard to read (there would just be a long line duplicating the ISA names in the source code). I think that checking the 2 most significant bits and the 2 least significant bits should be good enough.
I am not suggesting to test each bit individually but suggesting to prepare an input that would test all of them. I.e. a case where all bits are set.
I think that makes sense because we want to test printing of all of the bits actually.
That is how we test them in LLD and in tests for tools like llvm-objdump.
Not sure this is the best example, but we have lld/test/ELF/dt_flags.s in LLD and its idea is to test we set all the dynamic entries flags for
the command line passed. The same is here - just show we can dump the flags correctly.
I cross-referenced the values from https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf