[PATCH] D59007: [WebAssembly] Use named operands to identify loads and stores

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Nice! I'm learning more tablegen stuff everytime I see your CL 👍I have a few questions:



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp:74
+  if (unsigned(WebAssembly::getNamedOperandIdx(
+          MI.getOpcode(), WebAssembly::OpName::addr)) == FIOperandNum) {
+    unsigned OffsetOperandNum = WebAssembly::getNamedOperandIdx(
----------------
Nit: how about making it a variable as you did for `OffsetOperandNum` below, like
```
unsigned AddrOperandNum = WebAssembly::getNamedOperandIdx(
          MI.getOpcode(), WebAssembly::OpName::addr));
if (AddrOperandNum == FIOperandNum) {
  ...
```


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp:86
     }
   }
 
----------------
While this is a lot more general and better than the previous code and the fix looks correct, I can't find any bulk memory instructions in its td file that has `addr` operands. I don't think the bulk memory test cases enter this if block. Is that intended?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblySetP2AlignOperands.cpp:94
+  // Not changed
+  return false;
 }
----------------
Why not changed? I see the previous code didn't even touch `Changed`, but aren't we supposed set it to true when `if (P2AlignOpNum != -1)` holds?


================
Comment at: llvm/test/CodeGen/WebAssembly/bulk-memory.ll:147
+; 10 because in non-leaf function it must be 16-byte aligned, and
+; there is no special case for leaf functions.
+
----------------
- This sentence sounds like `__stack_pointer` global itself is bumped to a new location, which actually happens in non-leaf functions. But here we only read it to compute the frame index and don't modify `__stack_pointer` itself..?
- "in non-leaf function it must be 16-byte aligned, and there is no special case for leaf functions.": Why is it 16 bytes aligned and why is there no special case for leaf functions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59007