[PATCH] D80769: [WebAssembly] Adding 64-bit versions of all load & store ops.

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/test/CodeGen/WebAssembly/cpus.ll:4
 ; RUN: llc < %s -asm-verbose=false -mtriple=wasm32-unknown-unknown -mcpu=mvp 2>&1 | FileCheck %s
-; RUN: not --crash llc < %s -asm-verbose=false -mtriple=wasm64-unknown-unknown-wasm -mcpu=mvp 2>&1 | FileCheck %s --check-prefix=WASM64
+; RUN: llc < %s -asm-verbose=false -mtriple=wasm64-unknown-unknown-wasm -mcpu=mvp 2>&1 | FileCheck %s
 ; RUN: llc < %s -asm-verbose=false -mtriple=wasm32-unknown-unknown -mcpu=generic 2>&1 | FileCheck %s
----------------
aardappel wrote:
> tlively wrote:
> > dschuff wrote:
> > > aardappel wrote:
> > > > dschuff wrote:
> > > > > probably not to fix in this CL, but triple=wasm64 and -mcpu=mvp probably shouldn't be allowed.
> > > > I was actually thinking about that, but looking at the code that deals with the cpu flags (in WebAssembly.cpp) and couldn't see a good place to error out on this. Maybe in `setABI` ?
> > > > Also, what should the default cpu be if someone selects `wasm64` ?
> > > yeah I guess the feature flags are a bit entangled in the CPU. If we can it would be good to error on any cpu setting (or feature flag setting) that doesn't support memory64.
> > > my guess would be that the default cpu for wasm64 should just have the memory64 feature and no others. Not sure about the best place for the error (other than probably somewhere in clang). Perhaps @tlively has thought about this more recently than I have?
> > We have a bunch of errors for incompatible features and options in the driver code in clang already. That would probably be a good place for these checks, too.
> > 
> > As for the default cpu for the wasm64 triple, we could either do a cpu with 64-bit memory and no other features like `mvp`, or we could do a cpu that has 65-bit memory and all the small features that were standardized and widely implemented before it. Since there aren't many such features right now, though, I guess an mvp-like `mvp64` cpu would be best for now.
> Ok, where do I add this? :) Maybe ok as a followup?
oh and yes, I think it's fine for a followup and i mentioned above.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80769/new/

https://reviews.llvm.org/D80769