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

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp:74
+  if (unsigned(WebAssembly::getNamedOperandIdx(
+          MI.getOpcode(), WebAssembly::OpName::addr)) == FIOperandNum) {
+    unsigned OffsetOperandNum = WebAssembly::getNamedOperandIdx(
----------------
aheejin wrote:
> 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) {
>   ...
> ```
Good idea, will do.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp:86
     }
   }
 
----------------
aheejin wrote:
> 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?
Very much intended! The previous errors were due to bulk memory operations were entering this block, which tried to extract Offset operands that did not exist. Now they do not enter this block, which fixes the error.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblySetP2AlignOperands.cpp:94
+  // Not changed
+  return false;
 }
----------------
aheejin wrote:
> 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?
I had this question too, but I did not know the full context of what this return value means and how it is used, so I wanted to be conservative and not change the behavior of the code. It looks like setting `Changed` to true if we call `rewriteP2Align` doesn't break anything, so we can go with that.


================
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.
+
----------------
aheejin wrote:
> - 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?
Updated to reflect my findings from investigating `TransientStackAlignment`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59007