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

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

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 seen in bind. In that case, the GEP has been hoisted, but the load has not. In general the GEP could be in a different basic block, or even in the same basic block with an instruction that may not return (intrinsic, real function call, well-defined language-level exception, etc). You can insert the reltable.shift where the GEP is, and that probably makes sense given it serves (part of) the same purpose, but you *must* insert the actual reltable.intrinsic where the original load is, unless you've gone to great lengths to prove it's safe not to (which seems best left to the usual culprits like LICM).
> 
> IR test cases: https://godbolt.org/z/YMdaMrobE (bind is characterised by the first of the two functions)
@dim and @jrtc27 thank you for reporting it.
I see what's going wrong, and I uploaded a patch that fixes the issue by ensuring that the call to load.relative.intrinsic is inserted before the load, but not gep.
Please see https://reviews.llvm.org/D115571. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355