[PATCH] D112105: [ADT] Simplifying hex string parsing so it runs faster in debug modes.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

We don't usually optimize for debug performance - is there a particular case where this comes up that's worth prioritizing/changing code for (& is there a good heuristic we can use about which debug performance is going to matter and which isn't?)



================
Comment at: llvm/include/llvm/ADT/StringExtras.h:71
+  /* clang-format off */
+  static const int16_t LUT[256] = {
+    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
----------------
Can we drop the array dimension here and rely on the array contents to be the right size?


================
Comment at: llvm/include/llvm/ADT/StringExtras.h:72-87
+    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
+    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
+    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
+     0,  1,  2,  3,  4,  5,  6,  7,  8,  9, -1, -1, -1, -1, -1, -1,  // '0'..'9'
+    -1, 10, 11, 12, 13, 14, 15, -1, -1, -1, -1, -1, -1, -1, -1, -1,  // 'A'..'F'
+    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
+    -1, 10, 11, 12, 13, 14, 15, -1, -1, -1, -1, -1, -1, -1, -1, -1,  // 'a'..'f'
----------------
Does this need to be written out explicitly? How much does this/why does this change the performance (of what situation) significantly?

I guess it's enabling some optimization opportunities in some way by making the constant values more obvious? Perhaps we could write the code in some other way that'd satisfy that?

I wonder what it'd be like if 'LUT' were a constexpr std::array returned from a constexpr function that built it?


================
Comment at: llvm/unittests/ADT/StringExtrasTest.cpp:93
 
-  std::string InvalidStr = "A5ZX";
+  std::string InvalidStr = "A50\xFF";
   std::string IgnoredOutput;
----------------
If this is purely a performance improvement - what's motivating this test change?


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

https://reviews.llvm.org/D112105