[PATCH] D12002: Initial WebAssembly support in clang

Dan Gohman via cfe-commits cfe-commits at lists.llvm.org
Thu Sep 3 15:54:58 PDT 2015


sunfishcode added inline comments.

================
Comment at: lib/Basic/Targets.cpp:7643-7649
@@ +7642,9 @@
+  case llvm::Triple::wasm64:
+    // Until specific variations are defined, don't permit any.
+    if (!(Triple == llvm::Triple("wasm64-unknown-unknown")) ||
+        (!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:
> sunfishcode wrote:
> > echristo wrote:
> > > Ditto.
> > > 
> > > (I said this just below, but it seems to have gotten munged in the newer version)
> > I actually did see your comment and updated the code accordingly. It now does a positive test, `Triple == llvm::Triple("wasm64-unknown-unknown")`, which is simpler than what it did before.
> > 
> > However, it's also doing additional tests, because the Triple class's operator== doesn't distinguish between an Unknown that was actually "unknown" or an unknown that was some other string. Until we figure out what "vendor", "OS", and "environment" variations of wasm make sense (if any), we want to avoid dealing with accidental alternate triples.
> I think this is a lot of overkill here, no other target cares about this so why should the wasm target? If it's that important maybe fix Triple?
This Triple issue is not important enough to hold up the rest of the patch for, so I just removed it. Thanks for the review!


Repository:
  rL LLVM

http://reviews.llvm.org/D12002





More information about the cfe-commits mailing list