[PATCH] D84026: [AMDGPU][MC] Added support of SP3 syntax for MTBUF format modifier

Thu Jul 23 13:52:28 PDT 2020

dp marked an inline comment as done.
dp added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:794
+  for (int Id = DFMT_MIN; Id <= DFMT_MAX; ++Id) {
+    if (DfmtSymbolic[Id] && Name == DfmtSymbolic[Id])
+      return Id;
----------------
arsenm wrote:
> dp wrote:
> > vpykhtin wrote:
> > > Should we use any map structures here and other places? I was always curious is there something lightweight for such cases, like presorted list of IDs with binary search on it.
> > I do not know if there is anything useful in llvm libraries. We could use binary search but I believe we won't get any benefit for short tables (say 10-15 elements or so). And most parser tables are short indeed.
> > 
> > A speedup due to binary search use will be noticeable for long tables. But how much could we gain in real-life scenarios?
> > 
> > I did a little experiment with this change for gfx10. I assembled 10.000.000 of mtbuf instructions with BUF_FMT_8_UNORM (which is the second element of UfmtSymbolic) and another 10.000.000 with BUF_FMT_32_32_32_32_FLOAT (which is the 79th element). The assembly time was 77 and 85 seconds respectively. So in the worst case we have 10% slowdown and 5% on the average - this is the price for using linear search. 
> > 
> > I believe mtbuf instructions won't take up more than 10% of real-life programs so the slowdown comparing with binary search is less than 0.5%. Is that acceptable?
> Could you use StringLiteral to cut down on the string compare time instead of the const char*s? They'll fail on the length compare
Thanks Matt, that’s a good idea!

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

https://reviews.llvm.org/D84026