[PATCH] D86644: [llvm-mca] Initial implementation of output serialization using JSON

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: llvm/test/tools/llvm-mca/JSON/X86/views.s:85-103
+# CHECK-NEXT:   "ResourcePressureView": {
+# CHECK-NEXT:     "ResourcePressureInfo": [
+# CHECK-NEXT:       {
+# CHECK-NEXT:         "Denominator": 1,
+# CHECK-NEXT:         "Numerator": 0
+# CHECK-NEXT:       },
+# CHECK-NEXT:       {
----------------
There are multiple problems here:

An instruction rarely consumes more than maybe three or four processor resource units during an entrie simulation. So, array `ResourcePressureInfo` is going to be very sparse (subtargets may declare more than 10 units).
In practice, 'Numerator' is going to be zero most of the times for most entries.

It is not clear how to reconstruct the resource pressure view from this json output.
In this example, there are 10 processor resource units. However, the number of actual resources declared in JSON array `Resources` is more than 10 (i.e. it contains names for both units and groups). The `ResourcePressureView` only cares about resource units.

When processing the content of the json file, this forces the user to skip resource groups, and assume that units always come first within array `Resources`. While this may be true now, we don't want to strongly rely on an implementation detail.

Another (minor) issue is that "Denominator" is currently set to 1. However, to compute the actual pressure value the user needs to do the following math:
` ( Numerator / Denominator) / Executions `.

This approach requires that we look elsewhere for the execution number (which is 100 in this example). Personally, I think it is more user friendly if we just do the math and stick a double in there (rather than having "Numerator" and "Denominator").
 
So, I suggest the following two changes:
 1. Only report entries where Numerator is different than zero.
 2. Each entry must also provide an instruction index and a resource index.
 3. Report the actual pressure value as a double (up to two digits after the dot), instead of field `Numerator` and `Denominator`.


Soomething like this:
```{
  "InstructionIndex": 0,
  "ResourceIndex" : 2,
  "Pressure": 1.00
} ```



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

https://reviews.llvm.org/D86644