wingo added inline comments.
================
Comment at: clang/lib/Basic/Targets/WebAssembly.h:150
: WebAssemblyTargetInfo(T, Opts) {
- resetDataLayout("e-m:e-p:32:32-i64:64-n32:64-S128");
+ resetDataLayout("e-m:e-p:32:32-i64:64-n32:64-S128-ni:1");
}
----------------
This change is already in D101608; rebase?
================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4083
EVT PtrVT = Ptr.getValueType();
+ EVT EltVT = PtrVT.getScalarType();
----------------
It turns out that all the changes here to `visitLoad` and `visitStore` are no
longer needed. The issue was that before we made `getPointerMemTy` virtual, it
was hard-coded to return an integer of whatever the pointer bit size was. But
for externref and funcref values, where we're using pointers in non-default
address spaces to represent opaque values, that's not what we want: an
externref "in memory" isn't an i32. Having made `getPointerMemTy` virtual and
returning externref / funcref for those values, now we avoid the zero-extension
paths, and even the ISD::ADD node doesn't cause us problems.
================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4257
+ // Skip ZExt or Trunc if a ValueVT is zero sized
+ if (!ValueVTs[i].isZeroSized() && MemVTs[i] != ValueVTs[i])
Val = DAG.getPtrExtOrTrunc(Val, dl, MemVTs[i]);
----------------
Can revert this too, as mentioned above.
================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1684
Type *Ty = VT.getTypeForEVT(Context);
- if (Alignment >= DL.getABITypeAlign(Ty)) {
+ if (!VT.isZeroSized() && Alignment >= DL.getABITypeAlign(Ty)) {
// Assume that an access that meets the ABI-specified alignment is fast.
----------------
This one I would think that we want to return true, that the access is "fast",
for access to zero-sized types.
================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:2347
+ return false;
+}
----------------
I just checked and it would seem that the `getPointerMemTy` change means that
this override no longer appears to be needed.
================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:15
multiclass TABLE<WebAssemblyRegClass rt> {
+ let mayLoad = 1 in
defm TABLE_GET_#rt : I<(outs rt:$res), (ins table32_op:$table),
----------------
I think you may need `hasSideEffects = 0` for these annotations to have an
effect.
================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:19
[],
"table.get\t$res, $table",
"table.get\t$table",
----------------
Not something for this patch, but this is certainly bogus: surely we mean
`table.get\t$table, $i` and we have an i32 index argument?
================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:59
+ (TABLE_SET_EXTERNREF i32:$table, i32:$idx, externref:$r)>,
+ Requires<[HasReferenceTypes]>;
+
----------------
Consider changing to use the approach used for `GLOBAL_GET` from the parent
commit, and folding these into the multiclass so that we don't have to repeat
the code for externref and funcref.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95425/new/
https://reviews.llvm.org/D95425
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
