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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan accepted this revision.
leonardchan added a comment.
This revision is now accepted and ready to land.

LGTM pending a few more comments. Should also give some time to let others respond if they have feedback.



================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:41-42
+
+  if (!dyn_cast<LoadInst>(GEP->use_begin()->getUser()))
+    return false;
+
----------------
Not needed here since you have the `isa` below.


================
Comment at: llvm/test/Transforms/RelLookupTableConverter/relative_lookup_table.ll:2-3
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+; RUN: opt < %s -passes=rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
----------------
We should also check some other `RUN`s to check that this isn't run on cases that return false in `shouldBuildRelLookupTables`: non-PIC, non-64-bit, other code model sizes, etc.


================
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(
----------------
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.


================
Comment at: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn:64
     "PromoteMemoryToRegister.cpp",
+    "RelLookupTableConverter.cpp"
     "SSAUpdater.cpp",
----------------
Good that you added this, but I think Nico has a bot that automatically updates these BUILD.gn files so manually updating them may not be necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355