tlively added a comment.
In D66035#2178105 <https://reviews.llvm.org/D66035#2178105>, @pmatos wrote:
> Please ignore my `.gitlab-ci.yml`. That's just an internal change that I got
> uploaded by mistake.
> I am looking to see this through and start discussion on this with the goal
> of landing it.
It's great to see this patch being picked up again!
> At the moment, for example this test crashes:
>
> struct {
> __attribute__((address_space(256))) char *a[0];
> } b;
> void c() {
> for (int d = 0; d < 10; d++)
> b.a[d] = 0;
> }
>
> This picked up from previous work by @vchuravy. I am still surprised by, and
> don't understand the reasoning behind, using a an i8 * for the externref
> representation. If anything I would expect to see i32 *.
> In any case, the test above crashes in loop vectorization in
> `TargetLowering.h` `getValueType` because externref is casted just fine to a
> pointer type and it shouldn't be since `externref` is supposed to be opaque.
I'm not aware of any particular reason to use i8* and I would expect i32* to
work as well. Certainly once LLVM has opaque pointers, we will want to take
advantage of those. What is this test case supposed to do? As far as I can
tell, it's not very meaningful to have reference types in a struct since they
can't be stored in memory. Is the zero assignment supposed to become a nullref
assignment?
> I would be keen to hear some comments and suggestions going forward on this.
I am curious about how you plan to use this functionality from frontends going
forward and what exact use cases you are aiming to support.
================
Comment at: clang/lib/Basic/Targets/WebAssembly.cpp:43
.Case("exception-handling", HasExceptionHandling)
+ .Case("reference-types", HasReferenceTypes)
.Case("bulk-memory", HasBulkMemory)
----------------
I would split the target-features changes out into a separate patch that we can
land right away. There is utility in having the feature flag available to be
put into the target-features section even if LLVM cannot itself emit reference
types usage yet.
================
Comment at: clang/lib/Basic/Targets/WebAssembly.cpp:183
+ HasReferenceTypes = true;
+ resetDataLayout("e-m:e-p:32:32-i64:64-n32:64-S128-ni:256");
+ continue;
----------------
It would be good to have a comment about what is different in this data layout
string. Also, is it really necessary to have a different data layout string
when reference types are enabled?
================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.td:169
def exnref: ValueType<0, 134>; // WebAssembly's exnref type
+def externref: ValueType<0, 135>; // WebAssembly's externref type
def token : ValueType<0 , 248>; // TokenTy
----------------
Do we need `funcref` here as well, or do we just use `externref` everywhere and
infer separate `funcref` types later?
================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4129
+ if (Offsets[i] != 0)
+ A = DAG.getNode(ISD::ADD, dl,
+ PtrVT, Ptr,
----------------
What is this change for?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66035/new/
https://reviews.llvm.org/D66035
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
