sunfish planned changes to this revision. sunfish marked 3 inline comments as done.
================ Comment at: lib/Basic/Targets.cpp:6935 @@ +6934,3 @@ + NoAsmVariants = true; + LongDoubleWidth = LongDoubleAlign = 64; + SuitableAlign = 128; ---------------- jfb wrote: > That's already the default? I thought I'd leave it in while we discuss whether this is really what we want. ================ Comment at: lib/Basic/Targets.cpp:6937 @@ +6936,3 @@ + SuitableAlign = 128; + RegParmMax = 0; // Disallow regparm + } ---------------- jfb wrote: > Already the default? Right. ================ Comment at: lib/Basic/Targets.cpp:6938 @@ +6937,3 @@ + RegParmMax = 0; // Disallow regparm + } + ---------------- jfb wrote: > `TLSSupported = false` for now. Since the MVP doesn't have threads at all, all variables are TLS :-). And after the MVP, we'll add threads with TLS. Is there a downside to leaving this on for now? It'll make the thread prototyping work easier. ================ Comment at: lib/Basic/Targets.cpp:6941 @@ +6940,3 @@ + void + getDefaultFeatures(llvm::StringMap<bool> &Features) const override final {} + bool setCPU(const std::string &Name) override final { ---------------- jfb wrote: > Not needed, since that's the default impl? We're going to put code in there before long anyway, so there's no harm in getting it ready. ================ Comment at: lib/Basic/Targets.cpp:6956 @@ +6955,3 @@ + + Builder.defineMacro("__REGISTER_PREFIX__", ""); + } ---------------- jfb wrote: > I'm not sure we need this. Does it just make porting easier? I don't know if we specifically need it; I just included it because lots of other popular targets have it. ================ Comment at: lib/Basic/Targets.cpp:6983 @@ +6982,3 @@ + const char *getClobbers() const override { return ""; } + bool isCLZForZeroUndef() const override { return false; } +}; ---------------- jfb wrote: > `final` for these two. Right. ================ Comment at: lib/Basic/Targets.cpp:6994 @@ +6993,3 @@ + +class WebAssembly32TargetInfo : public WebAssemblyTargetInfo { +public: ---------------- jfb wrote: > `final` This class is subclassed. ================ Comment at: lib/Basic/Targets.cpp:7015 @@ +7014,3 @@ + +class WebAssembly64TargetInfo : public WebAssemblyTargetInfo { +public: ---------------- jfb wrote: > `final` This one is too. ================ Comment at: lib/Driver/Tools.cpp:1559 @@ -1558,1 +1558,3 @@ +/// getWebAssemblyTargetCPU - Get the (LLVM) name of the WebAssembly cpu we are +/// targeting. ---------------- jfb wrote: > New comment style without `name -`. Right. ================ Comment at: test/CodeGen/wasm-arguments.c:91 @@ +90,2 @@ +// WEBASSEMBLY64: define void @f10(%struct.bitfield1* byval align 4 %bf1) +void f10(bitfield1 bf1) {} ---------------- jfb wrote: > TODO for vararg test? There's a TODO for "implement va_list properly" elsewhere, so we can add tests when we actually implement it. ================ Comment at: test/Driver/wasm32-unknown-unknown.cpp:51 @@ +50,3 @@ +void __wasm__defined() {} +#endif + ---------------- jfb wrote: > Also test `__wasm32__` (and 64 in the other file). test/Preprocessor/init.c covers this. ================ Comment at: test/Driver/wasm32-unknown-unknown.cpp:64 @@ +63,3 @@ +void _REENTRANTundefined() {} +#endif + ---------------- jfb wrote: > Test `__REGISTER_PREFIX__` if we do keep it. test/Preprocessor/init.c covers this. ================ Comment at: test/Preprocessor/init.c:8478 @@ +8477,3 @@ +// WEBASSEMBLY32:#define __GCC_ATOMIC_POINTER_LOCK_FREE 2 +// WEBASSEMBLY32:#define __GCC_ATOMIC_SHORT_LOCK_FREE 2 +// WEBASSEMBLY32:#define __GCC_ATOMIC_TEST_AND_SET_TRUEVAL 1 ---------------- jfb wrote: > `ATOMIC_*_LOCK_FREE` should always be `1` for "maybe". Clang is setting these automatically based on the value of MaxAtomicInlineWidth. Are you saying 32 is the wrong value for wasm32? This is perhaps a discussion to be had on the spec side. The value for wasm64 is also an interesting topic for the spec side. Repository: rL LLVM http://reviews.llvm.org/D12002 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits