This is an archive of the discontinued LLVM Phabricator instance.

[x86][inline-asm][llvm] accept 'v' constraint
ClosedPublic

Authored by coby on Sep 28 2016, 12:29 AM.

Details

Summary
  1. 'v' constraint for (x86) non-avx arch imitates the already implemented 'x' constraint, i.e. allows XMM{0-15} & YMM{0-15} depending on the apparent arch & mode (32/64).
  2. for the avx512 arch it allows [X,Y,Z]MM{0-31} (mode dependent)

This patch applies the needed changes to clang
clang patch: https://reviews.llvm.org/D25004

Diff Detail

Repository
rL LLVM

Event Timeline

coby updated this revision to Diff 72770.Sep 28 2016, 12:29 AM
coby retitled this revision from to [x86][inline-asm][llvm] accept 'v' constraint.
coby updated this object.
coby added reviewers: echristo, delena.
coby set the repository for this revision to rL LLVM.
coby added a subscriber: llvm-commits.
delena added inline comments.Sep 28 2016, 1:19 AM
lib/Target/X86/X86ISelLowering.cpp
32380

you can use FR32X if you have VLX. Otherwise take FR32. The same about other cases.

32385

line alignment

test/CodeGen/X86/inline-asm-avx512-v-constraint.ll
1 ↗(On Diff #72770)

you also should check knl.
Community asks to avoid -mcpu flag and use -march instead. (I don't know why)

coby updated this revision to Diff 72959.Sep 29 2016, 12:51 AM
coby marked 2 inline comments as done.

Addressing comments:

  1. Added more robust tests
  2. Refined EVEX-accessible registers selection
delena edited edge metadata.Sep 29 2016, 1:15 AM

In my opinion, you did not write enough tests. You should cover AVX512F more with zmm, ymm, xmm, multiple instructions.
You also should check error messages, when user puts wrong registers.

lib/Target/X86/X86ISelLowering.cpp
32036

does 'v' work without avx512?

32386

FR64 and FR64X will not work for i64 and i32 . Please add a test that checks this.

32406

you can remove Subtarget.hasAVX512(). VLX includes AV512

test/CodeGen/X86/inline-asm-avx512f-v-constraint.ll
3

-mattr -avx512vl is redundant, you can remove the second run.

coby updated this revision to Diff 74057.Oct 9 2016, 2:06 AM
coby edited edge metadata.

Addressing Elena's comments

coby updated this revision to Diff 74058.Oct 9 2016, 2:11 AM

Uploading correct diff

delena accepted this revision.Oct 9 2016, 2:13 AM
delena edited edge metadata.
This revision is now accepted and ready to land.Oct 9 2016, 2:13 AM
coby closed this revision.Oct 10 2016, 12:10 AM

Commitd to revision 283717