Page MenuHomePhabricator

[WebAssembly] Bulk memory intrinsics and builtins
ClosedPublic

Authored by tlively on Feb 4 2019, 9:50 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Feb 4 2019, 9:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 9:50 PM
aheejin added inline comments.Feb 11 2019, 9:02 AM
clang/include/clang/Basic/BuiltinsWebAssembly.def
29 ↗(On Diff #185235)
  • When do we use Ii instead of i?
  • Shouldn't we add the memory index field as well, even if that means a user always has to set it to 0? Are we gonna add a new builtin once multiple memories are available?
  • Shouldn't the segment index, segment offset, and the size operands be Ui because they cannot be negative?
30 ↗(On Diff #185235)

The same thing..

  • Shouldn't the segment index be Ui?
  • Shouldn't we add the memory index field as well?
clang/lib/CodeGen/CGBuiltin.cpp
13477 ↗(On Diff #185235)

Not sure if we can use llvm_unreachable here, because we can certainly reach here if a user uses this builtin with a non-const variable. In this file people often just used assert for user errors, which is not ideal either.

I haven't used it myself, but looking at the code, the recommended way to signal an error looks like to use CodeGenFunction::ErrorUnsupported function, as in here. We used llvm_unreachable for SIMD builtins too, but maybe we can fix it later.

13488 ↗(On Diff #185235)

Do we need to pass types here to make intrinsic names overloaded like llvm.wasm.memory.init.i32.i32.i8, unless this intrinsic also support operands of other types, which it doesn't? The same for data.drop builtin.

llvm/include/llvm/IR/IntrinsicsWebAssembly.td
119 ↗(On Diff #185235)
  • Why are the first two immediates anyint? The spec says the segment index is varuint32 (so that will be llvm_i32_ty here), and for the memory index I don't think we are ever gonna need something larger than i32 either, but maybe it is better to be clarified in the spec too though.
  • For the pointer type, I guess llvm_ptr_ty will be sufficient, unless we have multiple overloaded intrinsics of the same name.
121 ↗(On Diff #185235)
  • Isn't the pointer argument number not 1 but 2?
  • I guess this should have IntrHasSideEffects as well, as in data.drop?
  • I don't know much how they are handled differently in compilation, but because we can access some other memory, which holds the segment part, during execution, how about IntrInaccessibleMemOrArgMemOnly instead of IntrArgMemOnly?
122 ↗(On Diff #185235)

I told you we needed this, but on second thought, because we don't really use MachineMemOperand, maybe we don't need it..? :$ The standard memcpy/memmove/memset intrinsics don't have it either. So if the code runs ok without these I think we can delete it. The same for data.drop intrinsic. Sorry for the incorrect info and confusion.

125 ↗(On Diff #185235)

The same, llvm_i32_ty

Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 9:02 AM
tlively updated this revision to Diff 186393.Feb 11 2019, 9:06 PM
tlively marked 12 inline comments as done.
  • Address comments
    • Use Ui in builtin signatures
    • Remove unnecessary intrinsic polymorphism
    • Tweak intrinsic properties
clang/include/clang/Basic/BuiltinsWebAssembly.def
29 ↗(On Diff #185235)

We use Ii instead of i when the argument needs to be a compile time constant because it is encoded as a literal in the corresponding instruction.

I was imagining that we could add a new builtin once multiple memories are available, but perhaps it would be better to add an argument to the builtin now and error out if it is not zero. I will do that.

Yes, Ui seems reasonable.

30 ↗(On Diff #185235)
  • Done
  • This intrinsic doesn't have a memory index, since it doesn't write anything into memory.
clang/lib/CodeGen/CGBuiltin.cpp
13477 ↗(On Diff #185235)

llvm_unreachable is appropriate here because non-constant expressions will have been caught earlier by the type checking.

A follow-up PR updating SIMD intrinsics to use Ui and appropriate error handling sounds good.

13488 ↗(On Diff #185235)

Fixed. I had been copying llvm.memcpy, but I agree it is better to not be polymorphic.

llvm/include/llvm/IR/IntrinsicsWebAssembly.td
121 ↗(On Diff #185235)
  • Yes, good catch.
  • Ok, I'm not sure it's necessary but it can't hurt.
  • I'm not sure this is necessary either, but again it can't hurt. Better safe than sorry.
122 ↗(On Diff #185235)

No problem!

tlively updated this revision to Diff 186504.Feb 12 2019, 10:12 AM
  • Update tests to check full polymorphism of llvm.memcpy and llvm.memmove
aheejin accepted this revision.Feb 12 2019, 5:43 PM
aheejin added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
13477 ↗(On Diff #185235)

llvm_unreachable is appropriate here because non-constant expressions will have been caught earlier by the type checking.

Oh, does Ii ensures that? That's cool...

This revision is now accepted and ready to land.Feb 12 2019, 5:43 PM
This revision was automatically updated to reflect the committed changes.