[PATCH] D63633: [WebAssembly] Fix p2align in assembler.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel added inline comments.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:716
+        auto &Op0 = Inst.getOperand(0);
+        if (Op0.getImm() == -1) Op0.setImm(Align);
+      }
----------------
sbc100 wrote:
> aardappel wrote:
> > sbc100 wrote:
> > > Does llvm style allow this?  clang-format?
> > > 
> > > Is there some way we can tell if an opcode requires alignment?   Calling GetDefaultP2AlignAny and checking for -1 seems bit hacky.   Assuming I understand what happing I think this code would be more readable as:
> > > 
> > > ```
> > > if (requiresAlignment() && !hasAlignment) {
> > >   setImm(getDefaultAlign())
> > > }
> > > ```
> > Clang-formatted.
> > It is mildly-hacky, but I didn't want to duplicate the gigantic switch in `GetDefaultP2AlignAny`. Right now `GetDefaultP2AlignAny` is the source of truth for alignment in one place, which is also kinda nice.
> On I didn't see that we have already calculated `IsLoadStore`.. can we just use that?
`IsLoadStore` is a bit of a hack that I would prefer to remove, rather than rely more on.  I'd also have the get the instruction name in `MatchAndEmitInstruction` and repeat the same parsing there, so I think that will make the code generally worse.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:353
                        InstName.startswith("store") ||
                        InstName.startswith("atomic");
     if (IsLoadStore) {
----------------
aheejin wrote:
> Can atomic operations take `p2align`? Looks like alignments for atomic instructions are [[ https://github.com/WebAssembly/threads/blob/master/proposals/threads/Overview.md#spec-changes | predefined ]]. 
They are still stored in the encoding, and the tablegen def doesn't seem to make a difference between atomic and normal loads as to wether p2align can be changed, but yeah, the spec seems to indicate they are fixed. I can force atomics to use natural alignment for now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63633