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

Gulfem Savrun Yeniceri via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 16 18:10:40 PDT 2021


gulfem marked 4 inline comments as done.
gulfem added inline comments.


================
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())
----------------
hans wrote:
> 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.
I actually ran into the exact case that you described during testing, where a function that uses a switch gets inlined into multiple call sites :)
This is only to simplify the analysis, and I now added a TODO section to explore that later.


================
Comment at: llvm/test/Transforms/RelLookupTableConverter/switch_relative_lookup_table.ll:39
+; ; Relative switch lookup table for strings
+define i8* @string_table(i32 %cond) {
+  ; FNOPIC-LABEL: @string_table(
----------------
leonardchan wrote:
> It looks like this test case isn't much different from `string_table` in `relative_lookup_table.ll`? If so, then this file could be removed.
I renamed this test case to no_relative_lookup_table.ll that checks the cases where relative lookup table should not be generated like in non-pic mode, medium or large code models, and 32 bit architectures, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355



More information about the cfe-commits mailing list