Add new pass LowerRefTypesIntPtrConv to generate trap
instruction for an inttoptr and ptrtoint of a reference type instead
of erroring, since calling these instructions on non-integral pointers
has been since allowed (see ac81cb7e6).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Unfortunately, I am still getting some terminator issues. I have a feeling my basic block splitting is not working properly. Still need to investigate further. Not ready to land.
Another approach would be to lower to DEBUG_UNREACHABLE, which is the normal unreachable instruction but not modeled as a terminator (see WebAssemblyInstrControl.td). I imagine you could use more standard ISel patterns for that lowering rather than introducing a separate pass.
Didn't know about DEBUG_UNREACHABLE, thanks. However, I notice this is too low in the compiler. I need to generate IR instructions and DEBUG_UNREACHABLE seems to be a machine instruction. Still don't think however that using normal patterns would work. The issue has to do with the fact that inttoptr and ptrtoint instructions are lowered during ISel Lowering and don't reach pattern matching. So, once you go into SelectionDAGISel::runOnMachineFunction(MF);, the function visitPtrToInt will try to generate a TRUNCATE for a reference type which will assert because of the zero size. So anything we do here, should be done PreISel as far as I understand.
Oh right, that makes sense. I wonder if a custom DAG combine would be too late, too. At any rate, if you use the @llvm.debugtrap intrinsic in the IR replacement pass, it will lower to unreachable without needing to split any basic blocks.
Nice! LGTM with those cleanups applied.
llvm/lib/Target/WebAssembly/WebAssemblyLowerRefTypesIntPtrConv.cpp | ||
---|---|---|
64 | dyn_cast should be pretty cheap, so I think it would be ok to just unconditionally do this second dyn_cast as well. | |
65–68 | It's probably worth making a isRefType helper that checks both of these conditions. |
I should have definitely used debugtrap instead of trap, which is still a terminator and expensive checks at the machine code level checked that there was a terminator in the middle of a basic block. *sigh*. Replacing with debugtrap as suggested by @tlively
dyn_cast should be pretty cheap, so I think it would be ok to just unconditionally do this second dyn_cast as well.