This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Refactor load ISel tablegen patterns into classes
ClosedPublic

Authored by dschuff on Aug 31 2017, 11:12 AM.

Details

Summary

Not all of these will be able to be used by atomics because tablegen, but it
still seems like a good change by itself.

Diff Detail

Repository
rL LLVM

Event Timeline

dschuff created this revision.Aug 31 2017, 11:12 AM

As an overall comment, this is awesome. Big fan of reducing syntactic noise.

lib/Target/WebAssembly/WebAssemblyInstrAtomics.td
27 ↗(On Diff #113438)

Is this actually different from LoadPatNoOffset? I'd think you could pass load or atomic_load as the node parameter and it should work either way.

34 ↗(On Diff #113438)

These look like they could use an ALoadPatImmOff

Also technically this is defining additional patterns, and isn't covered by the commit message. I'm ok with that though.

48 ↗(On Diff #113438)

What are these non-atomic loads doing here? Also aren't these already represented in WebAssemblyInstrMemory.td?

dschuff added inline comments.Aug 31 2017, 2:24 PM
lib/Target/WebAssembly/WebAssemblyInstrAtomics.td
27 ↗(On Diff #113438)

Alas... you'd think so. I certainly thought so. However I tried it, and then I read include/llvm/Target/TargetSelectionDAG.td and found that load (and the extending loads) are actually PatFrags, while the atomic loads are SDNodes, so they can't be freely mixed in templates like that. I feel like you could maybe make a PatFrag to wrap around the atomic loads and then use the other classes, but I don't know how to do that yet. Perhaps I'll try but I don't know whether it will be worth it or not, especially given we'll have different rules for selecting extending loads anyway (since there are no extending atomic loads), and that there are only 2 types instead of 4. So we'll see when we get more concrete about all of that.

34 ↗(On Diff #113438)

Ah yeah. I had actually intended to back this part of the CL out once I got deep enough into refactoring the other td file. I'll do that.

48 ↗(On Diff #113438)

Yeah this is just a paste that I had before I got down the other rabbit hole. will remove it too.

dschuff updated this revision to Diff 113475.Aug 31 2017, 2:29 PM
  • address comments: remove atomic rules with constant offsets
jgravelle-google accepted this revision.Aug 31 2017, 2:48 PM
This revision is now accepted and ready to land.Aug 31 2017, 2:48 PM
This revision was automatically updated to reflect the committed changes.