Page MenuHomePhabricator

Implementation of global.get/set for reftypes in LLVM IR
ClosedPublic

Authored by pmatos on Jan 26 2021, 12:50 AM.

Details

Summary

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.

Diff Detail

Unit TestsFailed

TimeTest
30 msx64 debian > Clang.CodeGen::align-wasm.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple wasm32-unknown-unknown -emit-llvm /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/align-wasm.c -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/align-wasm.c
70 msx64 debian > Clang.CodeGen::asan-constructor.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -fsanitize=address -emit-llvm -o - /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/asan-constructor.c | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/asan-constructor.c
210 msx64 debian > Clang.CodeGen::builtins-wasm.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple wasm32-unknown-unknown -target-feature +unimplemented-simd128 -target-feature +nontrapping-fptoint -target-feature +exception-handling -target-feature +bulk-memory -target-feature +atomics -flax-vector-conversions=none -O3 -emit-llvm -o - /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/builtins-wasm.c | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/builtins-wasm.c -check-prefixes WEBASSEMBLY,WEBASSEMBLY32
550 msx64 debian > Clang.CodeGen::ext-int-cc.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-gnu-linux -O3 -disable-llvm-passes -emit-llvm -o - /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/ext-int-cc.c | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/ext-int-cc.c --check-prefixes=LIN64
30 msx64 debian > Clang.CodeGen::no-prototype.c
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple wasm32 -emit-llvm /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/no-prototype.c -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/no-prototype.c
View Full Test Results (50 Failed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Fix else if and VT type as suggested by @wingo.

pmatos marked 3 inline comments as done.Apr 27 2021, 1:15 AM
pmatos added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1062

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.

1300

Sorry - I missed this. You're right, this should be MVT::Other.

pmatos updated this revision to Diff 340787.Apr 27 2021, 3:59 AM
pmatos marked an inline comment as done.

Make TABLE_SET node polymorphic. Deal with types through the Pats. One for funcref and one for externref.

pmatos added inline comments.Apr 27 2021, 7:41 AM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1062

Apologies I misunderstood your comment. We can make the node polymorphic and deal with the types in the Pat Frags. I will do that next.

This is looking really good!

llvm/lib/CodeGen/MachineOperand.cpp
1174 ↗(On Diff #340787)

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.

llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp
132–134 ↗(On Diff #340787)

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?

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1257

It would be good to mark these helper functions static.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
59–61

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.

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
-9

Unintentional whitespace change?

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
123

This check should incorporate the CPU as well using getSubtargetImpl(CPU, FS)->hasReferenceTypes().

llvm/test/CodeGen/WebAssembly/externref-undef.ll
13–14 ↗(On Diff #331366)

Ah, that makes sense.

sbc100 added inline comments.Apr 28 2021, 3:53 PM
llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp
132–134 ↗(On Diff #340787)

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.

wingo added inline comments.Apr 29 2021, 8:24 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4114 ↗(On Diff #340787)

Does this path actually fire for you? In my local tests, if you are loading from EXTERNREF_GLOBAL, then the EltVT ends up being i32.

pmatos updated this revision to Diff 342705.May 4 2021, 5:40 AM

Rebase this on D101608 from @wingo and clear table slot after call

pmatos updated this revision to Diff 342708.May 4 2021, 5:58 AM

Fix patch submitted to phab...

Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2021, 5:58 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
pmatos updated this revision to Diff 342712.May 4 2021, 6:11 AM

Fix patch set... again!

pmatos updated this revision to Diff 342718.May 4 2021, 6:55 AM

Fix broken merge of table ins reordering commit

pmatos updated this revision to Diff 342729.May 4 2021, 7:29 AM

Removed extraneous changes to previous patches

wingo added inline comments.May 4 2021, 7:46 AM
clang/lib/Basic/Targets/WebAssembly.h
150 ↗(On Diff #342708)

This change is already in D101608; rebase?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4083 ↗(On Diff #342729)

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.

4257 ↗(On Diff #342729)

Can revert this too, as mentioned above.

llvm/lib/CodeGen/TargetLoweringBase.cpp
1684 ↗(On Diff #342729)

This one I would think that we want to return true, that the access is "fast", for access to zero-sized types.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
2131

I just checked and it would seem that the getPointerMemTy change means that this override no longer appears to be needed.

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
15 ↗(On Diff #342729)

I think you may need hasSideEffects = 0 for these annotations to have an effect.

19 ↗(On Diff #342729)

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?

59 ↗(On Diff #342729)

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.

pmatos marked an inline comment as done.May 5 2021, 1:30 AM
tlively added inline comments.May 5 2021, 2:44 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
15 ↗(On Diff #342729)

I would be surprised if this were true!

19 ↗(On Diff #342729)

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.

llvm/test/CodeGen/WebAssembly/externref-globalget.ll
4 ↗(On Diff #331366)

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.

pmatos marked 7 inline comments as done.May 7 2021, 7:28 AM
pmatos added inline comments.
llvm/lib/CodeGen/MachineOperand.cpp
1174 ↗(On Diff #340787)

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

llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp
132–134 ↗(On Diff #340787)

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.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
59–61

Current set of patches should have addressed your concerns here.

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
15 ↗(On Diff #342729)

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.

19 ↗(On Diff #342729)

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.

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
123

What exactly do you mean here? Not sure what is the difference here. You mean adding something like : hasReferenceTypes(FS) || getSubtargetImpl(CPU, FS)->hasReferenceTypes()?

pmatos updated this revision to Diff 343685.May 7 2021, 8:04 AM
  • Simplifies some of code in the middle-end.
  • Removes setComdat and adds setWeak

Small improvements to address comments in review.

pmatos updated this revision to Diff 343695.May 7 2021, 8:48 AM

Making the linter happy...

wingo added inline comments.May 17 2021, 4:47 AM
llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
15 ↗(On Diff #342729)

Yeah I misunderstood, I was thinking that mayLoad and mayStore were a subset of the side effect annotations of hasSideEffects, which defaults to 1 effectively (grep for guessInstructionProperties). But no, the side effects modelled here are the disjoint bits mayLoad, mayStore, and hasSideEffects; mayRaiseFPException would appear to be a subset of hasSideEffects. Still, may best to explicitly set hasSideEffects to 1 for table.get and table.set just as documentation that they can trap if the index is out-of-bounds.

pmatos updated this revision to Diff 346411.May 19 2021, 5:00 AM

Rebase and update minor details and fix a few lint warnings...
Non-functional changes only.

@deadalnix @tlively @sbc100 anything missing here to get this landed?

It would be good to add dedicated tests for table.get and table.set as well.

llvm/lib/CodeGen/MachineOperand.cpp
1174 ↗(On Diff #340787)

Looks like this comment was cut off! I wonder if it would be better to skip printing opaque for now since we have no test of it/ Also, if another target comes along with a zero-sized type, I don't know whether "opaque" will necessarily describe it.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
507–508

Can you add the .wat of the code being generated here to the comment?

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
19 ↗(On Diff #342729)

I would still expect to see a register for the result and immediate inputs for the table symbol and the table slot index here.

llvm/test/CodeGen/WebAssembly/externref-undef.ll
13–14 ↗(On Diff #331366)

It would be good to add this explanation as a comment in the test file.

llvm/test/CodeGen/WebAssembly/funcref-call.ll
15 ↗(On Diff #346411)

Missing a table element index here.

pmatos added a comment.Tue, Jun 1, 8:07 AM

@tlively thanks for the comments. I am currently taking some time off but I will address your concerns upon my return.

pmatos updated this revision to Diff 350599.Tue, Jun 8, 7:06 AM

Fix details requested by @tlively in latest comemnts.

pmatos marked 6 inline comments as done.Tue, Jun 8, 7:13 AM

Hopefully we are close to landing this. :)

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
19 ↗(On Diff #342729)

Not sure I understood everything properly as the only thing missing was the index in the register based version. I added that now.

llvm/test/CodeGen/WebAssembly/funcref-call.ll
15 ↗(On Diff #346411)

I am not sure whether that's the case. According to the spec, table.set only gets a table. Two elements are popped from the stack: the index i32.const 0 and the value local.get 0.

tlively accepted this revision.Wed, Jun 9, 8:41 PM

Thanks for your patience through all the review! Let's get this landed :)

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
19 ↗(On Diff #342729)

Ah, sorry about that. I was confused and was thinking that the table index was an immediate, but of course you're right that it is not.

llvm/test/CodeGen/WebAssembly/funcref-call.ll
15 ↗(On Diff #346411)

Yep, sorry for the confusion.

This revision is now accepted and ready to land.Wed, Jun 9, 8:41 PM
This revision was landed with ongoing or failed builds.Thu, Jun 10, 1:08 AM
This revision was automatically updated to reflect the committed changes.
pmatos marked 2 inline comments as done.

Hi @pmatos, this has caused SVE related failures on our AArch64 bot (and probably others):
https://lab.llvm.org/buildbot/#/builders/43/builds/7187

Not an SVE expert myself but if you need help figuring it out I can find someone to help out I'm sure.

Also affecting riscv-v (riscvv?) so I've reverted the change.

https://lab.llvm.org/buildbot/#/builders/67/builds/3087

Also affecting riscv-v (riscvv?) so I've reverted the change.

https://lab.llvm.org/buildbot/#/builders/67/builds/3087

Apologies for the breakages and thanks for reverting. I will take a look at it and if I need help, I will get back to you.