This change implements new DAG notes GLOBAL_GET/GLOBAL_SET, and
lowering methods for load and stores of reference types from IR
globals. Once the lowering creates the new nodes, tablegen pattern
matches those and converts them to Wasm global.get/set.
|300 ms||x64 debian > LLVM.CodeGen/AArch64::dag-combine-insert-subvector.ll|
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/dag-combine-insert-subvector.ll -o /dev/null
|410 ms||x64 debian > LLVM.CodeGen/AArch64::inline-asm-constraints-bad-sve.ll|
Script: -- : 'RUN: at line 1'; not /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple=aarch64-none-linux-gnu -mattr=+sve -o - /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/inline-asm-constraints-bad-sve.ll 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/inline-asm-constraints-bad-sve.ll
|290 ms||x64 debian > LLVM.CodeGen/AArch64::named-vector-shuffles-sve.ll|
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -verify-machineinstrs < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/named-vector-shuffles-sve.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/named-vector-shuffles-sve.ll
|320 ms||x64 debian > LLVM.CodeGen/AArch64::spillfill-sve.ll|
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple=aarch64-none-linux-gnu -mattr=+sve -mattr=+bf16 < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/spillfill-sve.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/spillfill-sve.ll
|400 ms||x64 debian > LLVM.CodeGen/AArch64::split-vector-insert.ll|
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/split-vector-insert.ll -debug-only=legalize-types 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/split-vector-insert.ll --check-prefix=CHECK-LEGALIZATION
|View Full Test Results (301 Failed)|
|120 ↗||(On Diff #337379)|
The other names in this file seem to either use UPPER_SNAKE_CASE or CamelCase. Any reason these new one use lower_snake_case?
Missing newline at EOF here and in externref-ptrtoint.ll
I'm still not comfortable approving this patch with changes to llvm-c/, Bitcode/, or IR/. If those changes can't be backed out, this will require an RFC on llvm-dev, but I would be surprised if that were necessary.
Apparently getTypeForEVT in LowerArguments is only necessary to call the getABIAlignmentForCallingConv. I was thinking that since reference types really are pointers to different address spaces we could return their TypeForEVT as being int32. That will then return as the alignment for reference types the default alignment for int32. This should help with avoiding us touching middle end code. I will experiment in this direction.
|120 ↗||(On Diff #337379)|
should be just "if"
Should this be TABLE_SET_FUNCREF and not just TABLE_SET ? Given that we're producing a DAG node that then needs to be selected, I would think the type-specific stuff could happen in the pattern-matching.
Pretty sure this comment still applies
Returning a ref.null or an uninitialized externref would make more sense if the return type would be %externref. However, in this case this is an %extern value, which really is an opaque type and should never really happen.
In this case, I think we cannot generate a TABLE_SET. I think the TABLE_SET you are referring to is the machine instruction defined in the backend which is not matched from here. From here, you can only match the nodes you create and that's WebAssemblyISD::TABLE_SET_FUNCREF. I am still not totally clear why this is, but I never had any luck generating the machine instructions defined in the backend directly from lowering so there might be a way but it's not straightforward. So we generate here the custom machine node we created and then we pattern match that in tablegen to generate the instruction.
Sorry - I missed this. You're right, this should be MVT::Other.
This is looking really good!
Is there a test that demonstrates this printing? Also, I'm not sure specifically calling out zero-sized operands in the text dump is that useful. Can zero-sized operands still be given an alignment? If so, it might even be good to continue printing the alignment for them.
Is this necessary given that the symbol is set to be undefined? Perhaps it would be better to make it defined here and also set comdat so that the linker doesn't need to do anything special to make sure it exists in the final binary. (Or possibly I'm misunderstanding what these settings mean!) @sbc100 wdyt?
It would be good to mark these helper functions static.
Why are the address spaces for globals integral? It doesn't make sense to do arithmetic on the "address" of a global afaict. Does using getelementptr to index tables in the global address space require the address space to be integral? If so, that might be a good reason to make the address space integral, but it might also be a good reason to put tables and globals in different address spaces instead. Also, IMO it would be better to use just a single address space for all globals; that way we could support globals with other types like i32, i64, etc. without even more address spaces.
|1 ↗||(On Diff #340787)|
Unintentional whitespace change?
This check should incorporate the CPU as well using getSubtargetImpl(CPU, FS)->hasReferenceTypes().
Ah, that makes sense.
I think don't want this here. Firstly I think this will mark the symbol as being a comdat group symbol .. which is not the same thing as being part of a comdat group ( which I think is that is indented here). Secondly, I don't think it makes sense for an undefined symbol to be part of a comdat group. I think what you want here, if the symbol was defined is setWeak(). Given that is not defined I dont think you even want that.
|150 ↗||(On Diff #342708)|
This change is already in D101608; rebase?
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.
Can revert this too, as mentioned above.
This one I would think that we want to return true, that the access is "fast", for access to zero-sized types.
I just checked and it would seem that the getPointerMemTy change means that this override no longer appears to be needed.
I think you may need hasSideEffects = 0 for these annotations to have an effect.
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?
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.
I would be surprised if this were true!
Agree about the i32 index argument, but it is correct to have the result as part of the string for the register-based output format.
I forget if we had or resolved this conversation elsewhere already, but I would expect clang to emit pointers in the correct address spaces, yes. The SVE folks had to go though multiple years of effort to add scalable vectors to LLVM IR because there was no existing way to model their semantics in IR such that they could implement all the vectorization optimizations they wanted. We are lucky that we can get away with using address space pointers directly without having to add new types to the IR.
The reason I changed this here is due to the call of getSize() crashing for zeroSized arguments. I have not added test for the printing. I don't think zero-sized operands can still be given an alignment. We are actually setting the alignment to
It seems I was mistaken - I thought that by setting it as Comdat, the linker would merge symbols from different compilation units if they existed. Maybe what I need is Weak then.
Current set of patches should have addressed your concerns here.
Why would this be the case? If I remember correctly, I added mayLoad and mayStore here so that lowering includes a chain. And this works without the need for hasSideEffects. Unless you think this is required for other reaons, but mayLoad works with it.
Not sure I understand why this should be $i instead of $table. Isn't $table represented as an index anyway? A table32_op is defined as Operand<i32> so I don't quite understand what we gain by changing this.
What exactly do you mean here? Not sure what is the difference here. You mean adding something like : hasReferenceTypes(FS) || getSubtargetImpl(CPU, FS)->hasReferenceTypes()?