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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
