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

Fri Jul 24 06:45:36 PDT 2020

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;
----------------
dp wrote:
> 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!
Replacing 'C' strings with StringLiteral improved performance dramatically. Assembly time for tests I described above is now identical (77-78 seconds). Using other elements from the end of the table did not change the results. Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84026