[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

Looking good. Just a few more comments on my end.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5740-5747
+    // Make sure the table index will not overflow when treated as signed.
+    IntegerType *IT = cast<IntegerType>(Index->getType());
+    uint64_t TableSize =
+        RelOffsetArray->getInitializer()->getType()->getArrayNumElements();
+    if (TableSize > (1ULL << (IT->getBitWidth() - 1)))
+      Index = Builder.CreateZExt(
+          Index, IntegerType::get(IT->getContext(), IT->getBitWidth() + 1),
----------------
Nit: Since this seems to be the exact same as the start of the `ArrayKind` case, it might be cleaner to move this into a lambda or function.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5753
+    Value *Offset =
+        Builder.CreateMul(Index, Builder.getInt32(4), "switch.reltable.mul");
+
----------------
It's possible during ISel that this could be lowered to a left shift by 2, but I think it might be better to explicitly use left shift instead here.


================
Comment at: llvm/test/Transforms/SimplifyCFG/X86/switch_to_relative_lookup_table.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -simplifycfg -switch-to-lookup=true -relative-switch-lookup-table=true -keep-loops=false -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
----------------
I believe another LUT enum value is `SingleValue` which indicates that the table would only have one element. In this case, it looks like the table shouldn't be emitted and the switch lookup should just emit a direct reference. It would be useful to also have a test to ensure this still happens when this is enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355