[PATCH] D139631: [WebAssembly][NFC] Add ComplexPattern for loads

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Mostly naming and formatting nits. By the way, are you planning to work on store patterns too? If so that'd be more consistent with this.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:296-298
+  // WebAssembly constant offsets are performed as unsigned with
+  // infinite precision, so we need to check for NoUnsignedWrap so
+  // that we don't fold an offset for an add that needs wrapping.
----------------
Can we wrap comments to 80 cols if possible? The same for other comment blocks too.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:317
+
+bool WebAssemblyDAGToDAGISel::SelectLoadOperands(MVT AddrTyp, unsigned ConstOpc,
+                                                 SDValue N, SDValue &Offset,
----------------
We don't seem to abbreviate `Type` as `Typ` in this file or other files in Wasm backend, so for consistency I suggest we just say `Type`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrMemory.td:96
+multiclass LoadPat<ValueType ty, SDPatternOperator kind, string Name> {
+      def : Pat<(ty (kind (LoadOps32 I32:$offset, I32:$dynamic))),
+           (!cast<NI>(Name # "_A32") 0,
----------------
aheejin wrote:
> I guess this doesn't affect the functionality, but we use this for offsets in other patterns too, so for consistency.
How about using `addr` in place of `dynamic` for consistency within this file? For other occurrences in the patterns and comments too.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrMemory.td:96-99
+      def : Pat<(ty (kind (LoadOps32 I32:$offset, I32:$dynamic))),
+           (!cast<NI>(Name # "_A32") 0,
+                                     I32:$offset,
+                                     I32:$dynamic)>,
----------------
I guess this doesn't affect the functionality, but we use this for offsets in other patterns too, so for consistency.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrMemory.td:102-104
+      def : Pat<(ty (kind (LoadOps64 I64:$offset, I64:$dynamic))),
+           (!cast<NI>(Name # "_A64") 0,
+                                     I64:$offset,
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139631