Page MenuHomePhabricator

[SelectionDAG] Legalize vaargs that require vector splitting
ClosedPublic

Authored by luke on Apr 16 2019, 1:36 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

luke created this revision.Apr 16 2019, 1:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 1:36 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon edited reviewers, added: craig.topper; removed: delena.May 21 2019, 5:28 AM

Please regenerate the patch diff with context

luke updated this revision to Diff 200470.May 21 2019, 5:47 AM

Add full context to the patch

tpr added a comment.May 21 2019, 6:35 AM

LGTM, but I don't think I know the legalization code well enough to approve this.

RKSimon added inline comments.May 21 2019, 8:23 AM
llvm/test/CodeGen/X86/legalize-vaarg.ll
1 ↗(On Diff #200470)

Please regenerate this with update_llc_test_checks.py

efriedma added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1860 ↗(On Diff #200470)

This probably doesn't match how the vector will actually be passed on a big-endian target. Endianness doesn't affect the ordering of the elements of a vector in LLVM. (See also http://llvm.org/docs/BigEndianNEON.html .)

luke updated this revision to Diff 200759.May 22 2019, 8:25 AM

Run update_llc_test_checks.py

luke marked an inline comment as done.May 22 2019, 8:26 AM
luke marked an inline comment as done.May 23 2019, 2:36 AM
luke added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1860 ↗(On Diff #200470)

Ok, so from what I understand then it should be fine to just remove this swap?

RKSimon added inline comments.May 23 2019, 3:30 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1860 ↗(On Diff #200470)

Yes, but we need test coverage - copying legalize-vaarg.ll as a powerpc test with le and be targets should be straightforward?

luke updated this revision to Diff 200971.EditedMay 23 2019, 7:26 AM

Removed endianness swapping and added PowerPC BE/LE test: (I'm not familiar with it: does this look ok?)

Adding some PowerPC people to double check

llvm/test/CodeGen/PowerPC/legalize-vaarg.ll
46 ↗(On Diff #200971)

copy and paste?

luke marked an inline comment as done.May 23 2019, 7:37 AM

Adding some PowerPC people to double check

Thanks!

llvm/test/CodeGen/PowerPC/legalize-vaarg.ll
46 ↗(On Diff #200971)

Whoops, thanks! I added in this line but looks like update_llc_test_checks.py kept it, will remove

luke updated this revision to Diff 200974.May 23 2019, 7:39 AM

Remove stray CHECK

luke marked an inline comment as done.May 23 2019, 7:39 AM
shchenz added inline comments.May 26 2019, 11:58 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1858 ↗(On Diff #200974)

Why vaarg has alignment 0? I think it should be the result of getABITypeAlignment() for type of Hi? and also the alignment for Lo should not be right.

shchenz added inline comments.May 27 2019, 12:04 AM
llvm/test/CodeGen/PowerPC/legalize-vaarg.ll
3 ↗(On Diff #200974)

triple powerpc64le-unknown-linux-gnu for LE and powerpc64-unknown-linux-gnu for BE?

luke updated this revision to Diff 201539.May 27 2019, 9:01 AM

Get alignment from new vector type

luke marked 2 inline comments as done.May 27 2019, 9:03 AM
luke added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1858 ↗(On Diff #200974)

Updated: I presume they should both have the same alignment?

PowerPC case now LGTM. I think this also needs approval for x86 case?

luke marked 3 inline comments as done.May 28 2019, 1:41 AM
RKSimon accepted this revision.May 28 2019, 3:37 AM

LGTM - thanks

This revision is now accepted and ready to land.May 28 2019, 3:37 AM
luke added a comment.May 28 2019, 4:05 AM

@RKSimon @shchenz Thanks. I don't have commit rights, would someone else mind landing this instead?

RKSimon requested changes to this revision.May 28 2019, 5:25 AM

Building this locally with EXPENSIVE_CHECKS enabled causes a large number of warnings in the x86 test:

    • Bad machine code: Missing mayStore flag ***
  • function: test_large_vec_vaarg
  • basic block: %bb.0 (0x25e70f4da58)
  • instruction: %11:gr32 = MOV32rm %stack.0.args, 1, $noreg, 0, $noreg :: (load store 8 on %ir.args)
This revision now requires changes to proceed.May 28 2019, 5:25 AM
luke added a comment.May 31 2019, 2:27 AM

Building this locally with EXPENSIVE_CHECKS enabled causes a large number of warnings in the x86 test:

    • Bad machine code: Missing mayStore flag ***
  • function: test_large_vec_vaarg
  • basic block: %bb.0 (0x25e70f4da58)
  • instruction: %11:gr32 = MOV32rm %stack.0.args, 1, $noreg, 0, $noreg :: (load store 8 on %ir.args)

It looks like this happens with all vaarg expansions: The same assertions are thrown without vectors:

define i32 @test_large_vec_vaarg(i32 %n, ...) {
  %args = alloca i8*, align 4
  %x = va_arg i8** %args, i32
  ret i32 %x
}

As well as on the existing vaarg tests (2008-10-29-ExpandVAARG.ll).

luke added a comment.Jun 17 2019, 2:47 AM

@RKSimon the X86 vaarg codegen tests seem to be passing now with EXPENSIVE_CHECKS enabled

luke updated this revision to Diff 205008.Jun 17 2019, 2:48 AM

Rebase against master

RKSimon accepted this revision.Jun 18 2019, 2:52 AM

LGTM - cheers

This revision is now accepted and ready to land.Jun 18 2019, 2:52 AM
luke added a comment.Jun 18 2019, 3:58 AM

@RKSimon @shchenz I don't have commit access, can someone else commit this for me? Thanks

This revision was automatically updated to reflect the committed changes.