This is an archive of the discontinued LLVM Phabricator instance.

NeonEmitter: clean up prototype modifiers
ClosedPublic

Authored by t.p.northover on Oct 30 2019, 8:37 AM.

Details

Reviewers
jmolloy
efriedma
Summary

What does the modifier 'j' do on a vector type in arm_neon.td? I don't know; I don't think I ever knew; and I wouldn't be surprised if no-one knows.

So when implementing new intrinsics everyone just has to keep the magic table of modifiers open in a side-panel. Additionally, we're rapidly running out of letters that we can add when intrinsics with new and strange prototypes come along (I'm looking at you, dot product!). So unless we adopt the mathematicians' trick of starting on Greek and Hebrew alphabets once the Latin letters run out (is TableGen UTF-8 safe?), a solution is needed.

So, the main point of this patch is to allow multiple modifiers per type, and strip the list of modifiers down to something much more orthogonal, and hopefully intuitive. Some things I had to make a choice on to write the patch, and might warrant debate:

  • Using '1' for scalar is inconsistent with '2, '3', '4', but '0' seems weird to me.
  • Parens to group is a different system from the typespec it's next to (where modifiers precede a canonical base type).
  • Switching default from 'd' to '.' might be egregious, but I prefer it and we're rewriting the whole file anyway.

I've included a script which I used to automatically port our .td files, and should be equally useful for any downstream users with changes to these files (I suggest running it on the last downstream version before this change hit).

Diff Detail

Event Timeline

t.p.northover created this revision.Oct 30 2019, 8:37 AM

It looks like this patch contains a few other changes, besides the changes to the prototypes. In particular, the change to CGBuiltin.cpp, and there are a few new lines in the .td files that don't correspond to anything in the old versions. Is that accidental, or is it part of cleaning up the prototypes, somehow?

It looks like this patch contains a few other changes, besides the changes to the prototypes. In particular, the change to CGBuiltin.cpp, and there are a few new lines in the .td files that don't correspond to anything in the old versions. Is that accidental, or is it part of cleaning up the prototypes, somehow?

The extra .td lines are because just those 3 intrinsics used a fixed-width modifier ("give me half, no matter the input") with multiple sizes of input so there's no way to represent that in the new scheme and they need to be split up. Notice the integer ones are already split up because there was no corrresponding "give me int32_t" modifier. That change is actually already a separate NFC commit in my local repository and I'd commit it like that so that the script worked cleanly.

The CGBuiltin change follows from dropping the heuristic hasFloatingProtoModifier when deciding what type to pass to CGBuiltin for the intrinsics. This affected vmulx and the vcvt intrinsics. In vcvt's case I eventually decided to support them by moving to an explicit '!' modifier and special-casing conversion because they make good use of having signedness on the type they're given. I didn't revisit vmulx after that change, but I'd be inclined to leave it as it is; I kind of think it's unlikely someone implementing that now would make use of the ! modifier, which seems like a pretty rare requirement.

There are two other things that I think are pretty straightforward, but do clutter this patch so I'll split them out: removing the special behaviour of 'a' (it can be implemented in .td at a net -ve lines); and changing Type to use an enum instead of a series of bools. I'll upload new diffs and update this one.

The other

Separated off the two features I mentioned as D69715 and D69716.

LGTM with a couple nits.

clang/include/clang/Basic/arm_neon_incl.td
203

'd' is gone.

clang/utils/convert_arm_neon.py
1

Are you going to commit this script? If you are, probably makes sense to include some sort of date, so it's clear which change you're talking about, and when it makes sense to remove it from the tree.

efriedma accepted this revision.Nov 18 2019, 6:18 PM
This revision is now accepted and ready to land.Nov 18 2019, 6:18 PM
t.p.northover closed this revision.Nov 20 2019, 5:20 AM
t.p.northover marked 2 inline comments as done.

Thanks. Pushed it with those suggestions:

To github.com:llvm/llvm-project.git

c34478f5f6c7..3f91705ca54b  master -> master

It looks like this broke vcreate_u16 and friends. From http://lab.llvm.org:8011/builders/aosp-O3-polly-before-vectorizer-unprofitable/builds/1224/steps/build-aosp/logs/stdio :

external/skia/src/opts/SkBitmapProcState_filter_neon.h:53:42: error: C-style cast from scalar 'int' to vector 'uint16x4_t' (vector of 4 'uint16_t' values) of different size
    vres = vshrn_n_u16(vcombine_u16(tmp, vcreate_u16(0)), 8); // shift down result by 8
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
llvm.inst/lib/clang/10.0.0/include/arm_neon.h:4149:11: note: expanded from macro 'vcreate_u16'
  __ret = (uint16x4_t)(__p0); \
          ^
llvm.inst/lib/clang/10.0.0/include/arm_neon.h:24249:21: note: expanded from macro 'vshrn_n_u16'
  uint16x8_t __s0 = __p0; \
                    ^~~~

Sorry about the delay investigating this, your e-mail bypassed my inbox for some reason and I only noticed the issue when Hans reverted the change this afternoon. I'm looking into it now.