Page MenuHomePhabricator

[WebAssembly] Implement multivalue call_indirects
ClosedPublic

Authored by tlively on Feb 6 2020, 8:41 PM.

Details

Summary

Unlike normal calls, call_indirects have immediate arguments that
caused a MachineVerifier failure without a small tweak to loosen the
verifier's requirements for variadicOpsAreDefs instructions.

One nice thing about the new call_indirects is that they do not need
to participate in the PCALL_INDIRECT mechanism because their post-isel
hook handles moving the function pointer argument and adding the flags
and typeindex arguments itself.

Diff Detail

Event Timeline

tlively created this revision.Feb 6 2020, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2020, 8:41 PM

It looks this assumes D71484 and D71496 have not been reverted, right?

It looks this assumes D71484 and D71496 have not been reverted, right?

Correct, I will have to reland those patches.

aheejin added inline comments.Feb 13 2020, 6:36 PM
llvm/lib/CodeGen/MachineVerifier.cpp
1598

Can we change the line above

unsigned NumDefs = MCID.getNumDefs();

to

unsigned NumDefs = MO->getParent()->getNumDefs();

?

If this works, we don't need to handle this as an exception case. But because this is in target-independent code that affects all targets, some of which can have whatever weird instructions, so I'm not very sure if that will work. How about trying that and see if it works at least, and don't put too much effort on fixing it in case it does not work?

llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td
77

So this does not affect the capability to handle variadic use ops in later passes? (e.g. MachineOperand::isUse() correctly return true for uses?)

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
211

Nit: How about NumVariadicDefs to be a bit more consistent in naming?

234

Can we change this condition to I < MI->getNumOperands()?

llvm/lib/Target/WebAssembly/WebAssemblyUtilities.cpp
138

Not about this CL, but this actually looks incorrect, doesn't it? I guess this function was never used for call_indirects after all...

141

What's the difference between getNumDefs and getNumExplicitDefs? Do we have any implicit defs?

tlively updated this revision to Diff 244743.Feb 14 2020, 1:02 PM
tlively marked 5 inline comments as done.
  • Address comments
llvm/lib/CodeGen/MachineVerifier.cpp
1598

I got this working using unsigned NumDefs = MO->getParent()->getNumExplicitDefs();, but I'm not sure this would be a good change to make. The MachineVerifier is supposed to be verifying that each machine instruction matches its definition in its MCID. By changing NumDefs to be derived from the machine instruction rather than the MCID, we are changing MachineVerifier so that it instead validates instructions against themselves, which is a much less powerful check.

llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td
77

At the Machine IR layer, each operand individually knows whether it is a use or a def, so MachineOperand::isUse() and friends still work correctly. At the MC layer everything is horribly broken, but that's ok because we don't have registers there except for tests. This accounts for some extra care I had to take with MCOperands in the final patch in this sequence.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
234

This doesn't seem to work. I haven't really dug into why, but I could if you'd like.

llvm/lib/Target/WebAssembly/WebAssemblyUtilities.cpp
138

Yes, this is very wrong. I believe I ended up fixing it in a later patch in this sequence.

141

Yes, one example of an implicit def is ARGUMENTS.

aheejin accepted this revision.Feb 14 2020, 3:41 PM

Nice!

llvm/lib/CodeGen/MachineVerifier.cpp
1598

I think you're right.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
234

Yeah I think that's more general statement that we don't need to subtract NumVariadicDefs separately. (And this is not MachineVerifier so we are not loosening checks) Digging into why sounds OK, but I don't think we need to spend too much time there if it does not work.

llvm/lib/Target/WebAssembly/WebAssemblyUtilities.cpp
141

Then I wonder how the previous code for CALL worked, because it uses getNumDefs..?

This revision is now accepted and ready to land.Feb 14 2020, 3:41 PM
tlively marked 2 inline comments as done.Feb 18 2020, 12:47 PM
tlively added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
234

I'll add a new variable unsigned DescIndex = I - NumVariadicDefs; so we at least don't repeat that subexpression.

llvm/lib/Target/WebAssembly/WebAssemblyUtilities.cpp
141

It looks like this code is only used in analyzing call side effects in WebAssemblyRegStackify. If the retrieved callee op is not a global, the code conservatively assumes it has all kinds of side effects. So we were previously emitting correct but overly-conservative stackifications.

This revision was automatically updated to reflect the committed changes.