Page MenuHomePhabricator

[WebAssembly] Add new pass to lower int/ptr conversions of reftypes
ClosedPublic

Authored by pmatos on Jul 29 2021, 1:46 PM.

Details

Summary

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).

Diff Detail

Event Timeline

pmatos created this revision.Jul 29 2021, 1:46 PM
pmatos requested review of this revision.Jul 29 2021, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2021, 1:46 PM

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.

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.

pmatos updated this revision to Diff 363437.Aug 2 2021, 4:26 AM
pmatos edited the summary of this revision. (Show Details)

Generate a trap instruction to replace inttoptr and ptrtoint instead of unreachable

tlively accepted this revision.Aug 2 2021, 5:47 AM

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.

This revision is now accepted and ready to land.Aug 2 2021, 5:47 AM
pmatos updated this revision to Diff 363518.Aug 2 2021, 10:39 AM

Cleanup code as suggested by @tlively

This revision was landed with ongoing or failed builds.Aug 2 2021, 10:40 AM
This revision was automatically updated to reflect the committed changes.
pmatos reopened this revision.Aug 2 2021, 11:33 PM

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

This revision is now accepted and ready to land.Aug 2 2021, 11:33 PM