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

Derek Schuff via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 4 13:51:21 PDT 2020


dschuff added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrMemory.td:62
 // Basic load.
 // FIXME: When we can break syntax compatibility, reorder the fields in the
 // asmstrings to match the binary encoding.
----------------
aardappel wrote:
> dschuff wrote:
> > side note: since we are about to start depending even more on the asm syntax, and having users try to use it, it's probably getting to now-or-never on this fix.
> Is this about swapping offset & align? I could do that in a next commit, though personally this doesn't sound important to me. @sbc100 opinion?
Yes, it's about swapping offset and align. I don't have a strong opinion on that, other than that we should decide whether or not to do it, and remove/update the comment as applicable.

One argument for keeping things the same might be that offsets are much more common (at least when hand-writing) than alignments, and a format like `<align>:<offset>` might require an alignment when there otherwise might not need to be one.
Anyway, all of this dates back from when we just needed something "for now", even before we had the fully-stacked style and binary writer. So now's a good chance to revisit the design if we want.


================
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:
> 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?


================
Comment at: llvm/test/CodeGen/WebAssembly/load-ext.ll:6
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
----------------
aardappel wrote:
> dschuff wrote:
> > should the datalayout be removed here too?
> > I think it does make sense for many of these simple tests to use the target-defined datalayout (i.e. use the same IR for both 32 and 64) but we should be a bit careful with that, given that the frontend is going to generate actually-different IR for some cases.
> Yes, forgot to delete it here. I assumed it was better to rely on whatever is default for wasm32/64, but not familiar with under what conditions this could produce values other than the ones in `datalayout`.
In general everything should have an explicit datalayout chosen by the frontend (also the default datalayout for a target is AFAIK the same as what clang uses explicitly when there are no ABI-affecting cflags). But when there is no frontend and it's just tests, it gets a little more interesting, and it sometimes makes sense to have tests to double duty. I think it's fine to leave it out, as long as we are actually intending to run the test 2 different ways and we've verified that we are testing what we intend to test. Otherwise it should have an explicit datalayout just because it's a general default thing to do.


================
Comment at: llvm/test/MC/WebAssembly/wasm64.s:2
+# RUN: llvm-mc -triple=wasm64-unknown-unknown -mattr=+atomics,+unimplemented-simd128,+nontrapping-fptoint,+exception-handling < %s | FileCheck %s
+# Check that it converts to .o without errors, but don't check any output:
+# RUN: llvm-mc -triple=wasm64-unknown-unknown -filetype=obj -mattr=+atomics,+unimplemented-simd128,+nontrapping-fptoint,+exception-handling -o %t.o < %s
----------------
aardappel wrote:
> dschuff wrote:
> > Are we able to check the encoding of the instructions yet? See e.g. https://github.com/llvm/llvm-project/blob/master/llvm/test/MC/WebAssembly/atomics-encodings.s
> Yes, we should be able to add that to this test. Not sure what the point is though, since all load/store ops have exactly the same encoding as in wasm32. 
> 
> Frankly, this test doesn't test anything, since the only thing that changes is that load/store now accept 64-bit operands. We need to test that those are being emitted by LLVM (which we don't test here), and that that combination is accepted by verifiers (which we also don't test here, the assembler doesn't verify types, and happily constructs illegal instruction sequences).
> 
> What is new here (and is currently commented out pending new reloc types) is `i64.const label`, but we can't check the encoding of that, since at the .o level it will just be all zeroes.
Oh right I had forgotten that the encodings were all the same.


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

https://reviews.llvm.org/D80769





More information about the llvm-commits mailing list