[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Sorry for being unresponsive for a while, I got distracted by various bugs.

I skimmed this and it's looking great. Just added a few nit picks.



================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:32
+  // If lookup table has more than one user,
+  // do not generate a relative lookup table.
+  if (!GV.hasOneUse())
----------------
It would be better if the comment said why. I suppose the reason is we need to be sure there aren't other uses of the table, because then it can't be replaced.

But it would be cool if a future version of this patch could handle when there are multiple loads from the table which can all be replaced -- for example this could happen if a function which uses a lookup table gets inlined into multiple places.


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:47
+
+  // If the original lookup table is not dso_local,
+  // do not generate a relative lookup table.
----------------
It would be good with a "why" here too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355