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

gulfem added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:132 + // Place new instruction sequence after GEP. + Builder.SetInsertPoint(GEP); + Value *Index = GEP->getOperand(2); jrtc27 wrote: > This line causes the bug

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

jrtc27 added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:395 +/// in 64-bit achitectures where 32-bit offsets might not be enough. +if (TM.getCodeModel() == CodeModel::Medium || +TM.getCodeModel() == CodeModel::Large)

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

jrtc27 added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:132 + // Place new instruction sequence after GEP. + Builder.SetInsertPoint(GEP); + Value *Index = GEP->getOperand(2); This line causes the bug seen in bind. In

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

dim added subscribers: emaste, jrtc27, dim. dim added a comment. FWIW, this commit turned out to break the FreeBSD dns/bind916 port, see https://bugs.freebsd.org/259921. The short story is that the bind9 code on and after this line:

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

gulfem added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:73 + return false; + +// If an operand in the lookup table is not dso_local, pcc wrote: > In the version of the patch that you committed, you have this check

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

pcc added inline comments. Herald added a subscriber: ormris. Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:73 + return false; + +// If an operand in the lookup table is not dso_local, In the version of the patch that you committed,

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

gulfem added a comment. > I would say if it is going to take longer than the end of the week to fix the > issue, it might be nice to have it reverted so that other people's builds > continue to work (I know a few people who build with full LTO in a two stage > configuration every week).

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

nathanchance added a comment. In D94355#2717958 , @gulfem wrote: > @nathanchance do you prefer me to revert the patch first as it might take me > a while to investigate it? > Btw, I was able to reproduce the issue by following your steps. I would say if

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

gulfem added a comment. @nathanchance do you prefer me to revert the patch first as it might take me a while to investigate it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94355/new/ https://reviews.llvm.org/D94355

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

nathanchance added a comment. In D94355#2717813 , @gulfem wrote: > Thanks for the report @nathanchance, and I'm looking at it now. Thanks! > Is this failure from one of the bots? No, this was discovered by a user of my LLVM build script:

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

gulfem added a comment. > This patch breaks a two stage build with LTO: Thanks for the report @nathanchance, and I'm looking at it now. Is this failure from one of the bots? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94355/new/

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

nathanchance added a comment. This patch breaks a two stage build with LTO: $ git bisect log # bad: [f0bc2782f281ca05221d2f1735bbaff6c4b81ebb] [TTI] NFC: Remove unused 'OptSize' parameter from shouldMaximizeVectorBandwidth # good: [9829f5e6b1bca9b61efc629770d28bb9014dec45] [CVP]

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

gulfem added a comment. > FYI, I pushed the fix for the aarch64-coff issue now (D99572 > , rGd5c5cf5ce8d921fc8c5e1b608c298a1ffa688d37 > ) and > pushed another commit to remove the code for

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

mstorsjo added a comment. In D94355#2665532 , @gulfem wrote: >> Would you be ok with reverting this change until I can sort that out, or can >> we disable the pass for those targets until then? > > I will disable the pass for those targets for now. >

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

gulfem added a comment. > Would you be ok with reverting this change until I can sort that out, or can > we disable the pass for those targets until then? I will disable the pass for those targets for now. When the issue is resolved, I would like to enable it for those targets as well.

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

aeubanks added inline comments. Comment at: llvm/test/Other/opt-O3-pipeline-enable-matrix.ll:322-323 ; CHECK-NEXT: Simplify the CFG +; CHECK-NEXT: Relative Lookup Table Converter +; CHECK-NEXT: FunctionPass Manager ; CHECK-NEXT: Annotation Remarks

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

aeubanks added inline comments. Comment at: llvm/test/Other/opt-O3-pipeline-enable-matrix.ll:322-323 ; CHECK-NEXT: Simplify the CFG +; CHECK-NEXT: Relative Lookup Table Converter +; CHECK-NEXT: FunctionPass Manager ; CHECK-NEXT: Annotation Remarks

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

rnk added subscribers: aeubanks, rnk. rnk added inline comments. Comment at: llvm/test/Other/opt-O3-pipeline-enable-matrix.ll:322-323 ; CHECK-NEXT: Simplify the CFG +; CHECK-NEXT: Relative Lookup Table Converter +; CHECK-NEXT: FunctionPass Manager ; CHECK-NEXT:

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

mstorsjo added a comment. In D94355#2657881 , @mstorsjo wrote: > It looks like this is breaking the Windows/ARM(64) target - it doesn't > produce the right relative relocations for symbol differences. It can be > reproduced with a testcase like this: >

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

mstorsjo added a comment. It looks like this is breaking the Windows/ARM(64) target - it doesn't produce the right relative relocations for symbol differences. It can be reproduced with a testcase like this: $ cat test.s .text func1: ret func2: ret

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

arichardson added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:91 + ArrayType::get(Type::getInt32Ty(M.getContext()), NumElts); + GlobalVariable *RelLookupTable = new GlobalVariable( + M, IntArrayTy, LookupTable.isConstant(),

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

gulfem added inline comments. Comment at: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn:64 "PromoteMemoryToRegister.cpp", +"RelLookupTableConverter.cpp" "SSAUpdater.cpp", thakis wrote: > leonardchan wrote: > > Good that you added this,

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

thakis added inline comments. Comment at: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn:64 "PromoteMemoryToRegister.cpp", +"RelLookupTableConverter.cpp" "SSAUpdater.cpp", leonardchan wrote: > Good that you added this, but I think Nico

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG78a65cd945d0: [Passes] Add relative lookup table converter pass (authored by gulfem). Changed prior to commit: https://reviews.llvm.org/D94355?vs=331139=332445#toc Repository: rG LLVM Github

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

lebedev.ri accepted this revision. lebedev.ri added a comment. Thank you! Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391 + +if (!TM.getTargetTriple().isArch64Bit()) + return false; + gulfem wrote: > lebedev.ri wrote: > > gulfem wrote: >

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

gulfem added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391 + +if (!TM.getTargetTriple().isArch64Bit()) + return false; + lebedev.ri wrote: > gulfem wrote: > > lebedev.ri wrote: > > > 1. But all tests are using `x86_64`

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

lebedev.ri added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391 + +if (!TM.getTargetTriple().isArch64Bit()) + return false; + gulfem wrote: > lebedev.ri wrote: > > 1. But all tests are using `x86_64` triple? > > 2. This is

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

gulfem added a comment. @lebedev.ri do you have further comments? If not, I would like to submit this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94355/new/ https://reviews.llvm.org/D94355 ___

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

gulfem marked 2 inline comments as done. gulfem added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391 + +if (!TM.getTargetTriple().isArch64Bit()) + return false; + lebedev.ri wrote: > 1. But all tests are using `x86_64`

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

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

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

gulfem updated this revision to Diff 331139. gulfem added a comment. 1. Add no_relative_lookup_table.ll test case that checks the cases where relative lookup table should not be generated 2. Add comments about dso_local check and single use check Repository: rG LLVM Github Monorepo CHANGES

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

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

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

lebedev.ri added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391 + +if (!TM.getTargetTriple().isArch64Bit()) + return false; + 1. But all tests are using `x86_64` triple? 2. This is somewhat backwards. if the target wants

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

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:

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

gulfem marked 7 inline comments as done. gulfem added inline comments. Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:384 + bool shouldBuildRelLookupTables() { +const TargetMachine = getTLI()->getTargetMachine(); leonardchan wrote: > Sorry, I think

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

gulfem updated this revision to Diff 330405. gulfem added a comment. 1. Used IsConstantOffsetFromGlobal() function 2. Added this pass to the end of legacy pass manager pipeline 3. Moved all the tests under a new test directory Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

leonardchan added a comment. One thing that just occurred to me: do we also perhaps want to hide this behind a flag? Right now it's being added to various default optimization pipelines, so some users might be surprised if they suddenly see their lookup tables change (either compiler or user

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

gulfem updated this revision to Diff 328354. gulfem added a comment. Use TTI hook for target machine checks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94355/new/ https://reviews.llvm.org/D94355 Files:

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

gulfem added inline comments. Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:76-77 + + /// If lookup table has more than one user, + /// do not generate a relative lookup table. + if (!GlobalVar.hasOneUser()) leonardchan wrote: > What's the

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

gulfem added a comment. In D94355#2572553 , @leonardchan wrote: > It looks like you have everything setup for running on the new PM, but it > doesn't look like the pass is added anywhere in the new PM pipeline. Thank you very much for pointing that out

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

gulfem added a comment. > Thanks for pushing this forward! I think this will be a nice transformation > once all the details are worked out. Thank you very much for all of your wonderful constructive feedback! I learned much more about LLVM and IR internals. Appreciate all your help!

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

gulfem marked 17 inline comments as done. gulfem added inline comments. Comment at: clang/test/CodeGen/switch-to-lookup-table.c:2 +// Check switch to lookup optimization in fPIC and fno-PIC mode +// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fno-PIC -S -emit-llvm