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

Fri Jun 12 12:03:59 PDT 2020

aheejin 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>;

----------------
aardappel wrote:
> aheejin wrote:
> > aardappel wrote:
> > > 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.
> > I don't know why string concatenation will be specially ugly only here, given that we are doing it everywhere else? Is there something I'm missing that something is different here? (And `AStore` patterns too)
> > 
> > I'm not sure what would be the reason for grouping multiclass in most of other patterns but not some of them, that's all. If the reason is there's not enough entries, `BinRMW` and `BinTer` have the same number of entries with this one too.
> Just looked if I could make this happen.
> 
> Making ATOMIC_I generate 2 variants is not as easy as for WebAssembyLoad, since it takes full dag arguments, which have several components that would have to be replaced with !subst or whatever.. this would get tricky.
> 
> Adding a multi-class just for every pair of instructions would actually make the code bigger, and certainly less readable.
> 
> For the patterns, you're looking at changing this:
> ```
> class WaitPatNoOffset<ValueType ty, Intrinsic kind, WebAssemblyRegClass rc,
>                       NI inst> :
>   Pat<(i32 (kind rc:$addr, ty:$exp, I64:$timeout)),
>       (inst 0, 0, rc:$addr, ty:$exp, I64:$timeout)>;
> def : WaitPatNoOffset<i32, int_wasm_atomic_wait_i32, I32, ATOMIC_WAIT_I32_A32>;
> def : WaitPatNoOffset<i64, int_wasm_atomic_wait_i64, I32, ATOMIC_WAIT_I64_A32>;
> def : WaitPatNoOffset<i32, int_wasm_atomic_wait_i32, I32, ATOMIC_WAIT_I32_A32>;
> def : WaitPatNoOffset<i64, int_wasm_atomic_wait_i64, I32, ATOMIC_WAIT_I64_A32>;
> 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>;
> 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>;
> ```
> into:
> ```
> multiclass WaitPatNoOffset<ValueType ty, Intrinsic kind,
>                       string inst> {
>   def : Pat<(i32 (kind I32:$addr, ty:$exp, I64:$timeout)),
>             (!cast<NI>(inst#_A32) 0, 0, I32:$addr, ty:$exp, I64:$timeout)>;
>   def : Pat<(i32 (kind I64:$addr, ty:$exp, I64:$timeout)),
>             (!cast<NI>(inst#_A64) 0, 0, I64:$addr, ty:$exp, I64:$timeout)>;
> }
> defm : WaitPatNoOffset<i32, int_wasm_atomic_wait_i32, "ATOMIC_WAIT_I32">;
> defm : WaitPatNoOffset<i64, int_wasm_atomic_wait_i64, "ATOMIC_WAIT_I64">;
> defm : WaitPatNoOffset<i32, int_wasm_atomic_wait_i32, "ATOMIC_WAIT_I32">;
> defm : WaitPatNoOffset<i64, int_wasm_atomic_wait_i64, "ATOMIC_WAIT_I64">;
> ```
> If you think that is better, I'll go make similar changes below.
(Chatted offline and posting the summary here) I meant grouping patterns, and wasn't talking about making two variants of `ATOMIC_I` with dags and everything. This CL makes grouped patterns in WebAssemblyInstrSIMD.td but doesn't touch `SIMD_I` itself. That's what I meant in this file too.

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

https://reviews.llvm.org/D80769