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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked an inline comment as done.
aardappel added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrAtomics.td:128
+def : WaitPatNoOffset<i32, int_wasm_atomic_wait_i32, I64, ATOMIC_WAIT_I32_A64>;
+def : WaitPatNoOffset<i64, int_wasm_atomic_wait_i64, I64, ATOMIC_WAIT_I64_A64>;
 
----------------
aheejin wrote:
> aardappel wrote:
> > aheejin wrote:
> > > Patterns and instruction definitions in WebAssemblyInstrMemory.td are defined as multiclasses to group A32 and A64 versions. I think it's better to be consistent here too..? The same for all patterns and defs for `atomic.wait`, `atomic.notify` and `AStoreOffset`.
> > I tried to stay close to the original definitions. I do not feel the amount of duplication here is excessive to warrant trying to factor things out into more common definitions, given that trying to abstract things in tablegen leads to less readable results than in a normal programming language.
> I'm not sure the structure of original definition of these is different from that of InstrMemory.td. For `ImmOff` patterns, here we have
> 
> ```
> class WaitPatImmOff<ValueType ty, Intrinsic kind, PatFrag operand, NI inst> :    
>   Pat<(i32 (kind (operand I32:$addr, imm:$off), ty:$exp, I64:$timeout)),         
>       (inst 0, imm:$off, I32:$addr, ty:$exp, I64:$timeout)>;                     
> def : WaitPatImmOff<i32, int_wasm_atomic_wait_i32, regPlusImm, ATOMIC_WAIT_I32>; 
> def : WaitPatImmOff<i32, int_wasm_atomic_wait_i32, or_is_add, ATOMIC_WAIT_I32>;  
> def : WaitPatImmOff<i64, int_wasm_atomic_wait_i64, regPlusImm, ATOMIC_WAIT_I64>; 
> def : WaitPatImmOff<i64, int_wasm_atomic_wait_i64, or_is_add, ATOMIC_WAIT_I64>;
> ```
> 
> And in WebAssemblyInstrMemory.td we have
> ```
> class LoadPatImmOff<ValueType ty, PatFrag kind, PatFrag operand, NI inst> :      
>   Pat<(ty (kind (operand I32:$addr, imm:$off))), (inst 0, imm:$off, I32:$addr)>;                                                                             
> def : LoadPatImmOff<i32, load, regPlusImm, LOAD_I32>;                            
> def : LoadPatImmOff<i64, load, regPlusImm, LOAD_I64>;                            
> def : LoadPatImmOff<f32, load, regPlusImm, LOAD_F32>;                            
> def : LoadPatImmOff<f64, load, regPlusImm, LOAD_F64>;                            
> ...
> ```
> 
> And I don't think the amount of duplication is excessive either, but I suggested this more for consistency, because other patterns are grouped with multiclasses. `BinRMW` and `TerRMW` related patterns in this file are grouped as well, even though they have the same number of entries with these patterns.
Agree it would be more consistent, but to make the change in this particular example requires doing instruction string concatenation (which I had to do in other cases to reduce the amount of duplication), whereas here it would just make things worse. I can make the change if you feel strongly about it, but I don't think it actually makes anything better.


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

https://reviews.llvm.org/D80769