[PATCH] D12002: Initial WebAssembly support in clang

[Dan Gohman via cfe-commits]

sunfish added a comment.

Thanks for the review!


================
Comment at: include/clang/Basic/TargetCXXABI.h:166
@@ -148,1 +165,3 @@
 
+  /// \brief Are member functions differently aligned?
+  bool areMemberFunctionsAligned() const {
----------------
echristo wrote:
> Can you elaborate on the comment here as to what the alignment here means or something? It looks incomplete otherwise.
Ok, I'll add a longer comment.

================
Comment at: lib/Basic/Targets.cpp:6935-6941
@@ +6934,9 @@
+      if (Feature == "+simd128") {
+        SIMDLevel = std::max(SIMDLevel, SIMD128);
+        continue;
+      }
+      if (Feature == "-simd128") {
+        SIMDLevel = std::min(SIMDLevel, SIMDEnum(SIMD128 - 1));
+        continue;
+      }
+
----------------
echristo wrote:
> Might make sense to copy the x86 bits here?
The x86 bits don't handle "-" attributes, so it's not directly applicable.

================
Comment at: lib/Basic/Targets.cpp:7633-7658
@@ -7464,1 +7632,28 @@
   }
+  case llvm::Triple::wasm32: {
+    // Until specific variations are defined, don't permit any.
+    if (Triple.getSubArch() != llvm::Triple::NoSubArch ||
+        Triple.getVendor() != llvm::Triple::UnknownVendor ||
+        Triple.getOS() != llvm::Triple::UnknownOS ||
+        Triple.getEnvironment() != llvm::Triple::UnknownEnvironment ||
+        Triple.getObjectFormat() != llvm::Triple::UnknownObjectFormat ||
+        (!Triple.getVendorName().empty() &&
+         Triple.getVendorName() != "unknown") ||
+        (!Triple.getOSName().empty() && Triple.getOSName() != "unknown") ||
+        (Triple.hasEnvironment() && Triple.getEnvironmentName() != "unknown"))
+      return nullptr;
+    return new WebAssemblyOSTargetInfo<WebAssembly32TargetInfo>(Triple);
+  }
+  case llvm::Triple::wasm64: {
+    // Until specific variations are defined, don't permit any.
+    if (Triple.getSubArch() != llvm::Triple::NoSubArch ||
+        Triple.getVendor() != llvm::Triple::UnknownVendor ||
+        Triple.getOS() != llvm::Triple::UnknownOS ||
+        Triple.getEnvironment() != llvm::Triple::UnknownEnvironment ||
+        Triple.getObjectFormat() != llvm::Triple::UnknownObjectFormat ||
+        (!Triple.getVendorName().empty() &&
+         Triple.getVendorName() != "unknown") ||
+        (!Triple.getOSName().empty() && Triple.getOSName() != "unknown") ||
+        (Triple.hasEnvironment() && Triple.getEnvironmentName() != "unknown"))
+      return nullptr;
+    return new WebAssemblyOSTargetInfo<WebAssembly64TargetInfo>(Triple);
----------------
echristo wrote:
> Seems overly complicated? Maybe just a positive test?
Good idea. I'll do that.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:824
@@ +823,3 @@
+  if (getTarget().getCXXABI().areMemberFunctionsAligned()) {
+    // C++ ABI requires 2-byte alignment for member functions.
+    if (F->getAlignment() < 2 && isa<CXXMethodDecl>(D))
----------------
echristo wrote:
> Update the comment?
Ok, I'll write a bit more here.

================
Comment at: lib/Driver/ToolChains.cpp:3853-3873
@@ +3852,23 @@
+
+bool WebAssembly::IsMathErrnoDefault() const { return false; }
+
+bool WebAssembly::IsObjCNonFragileABIDefault() const { return true; }
+
+bool WebAssembly::UseObjCMixedDispatch() const { return true; }
+
+bool WebAssembly::isPICDefault() const { return false; }
+
+bool WebAssembly::isPIEDefault() const { return false; }
+
+bool WebAssembly::isPICDefaultForced() const { return false; }
+
+bool WebAssembly::IsIntegratedAssemblerDefault() const { return true; }
+
+// TODO: Support Objective C stuff.
+bool WebAssembly::SupportsObjCGC() const { return false; }
+
+bool WebAssembly::hasBlocksRuntime() const { return false; }
+
+// TODO: Support profiling.
+bool WebAssembly::SupportsProfiling() const { return false; }
+
----------------
echristo wrote:
> No generic defaults here? Might also make sense to have these all inline if they're just return true/return false.
All these functions are returning non-default values or are overriding pure virtual functions.

And they're outline because they're virtual overrides, so there's little optimization advantage to defining them in the header. And it's what most of the other toolchain definitions do most of the time.

================
Comment at: lib/Driver/Tools.cpp:1564-1569
@@ +1563,8 @@
+
+#ifdef __wasm__
+    // Handle "native" by examining the host. "native" isn't meaningful when
+    // cross compiling, so only support this when the host is also WebAssembly.
+    if (CPU == "native")
+      return llvm::sys::getHostCPUName();
+#endif
+
----------------
echristo wrote:
> I really dislike the idea of an ifdef here for this behavior. Can you explain some more? :)
As we discussed on IRC, I'll remove this code for now.


Repository:
  rL LLVM

http://reviews.llvm.org/D12002