This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Custom Lowering BUILD_VECTOR for v2i64 for P7 as well
ClosedPublic

Authored by jsji on Jul 7 2021, 2:59 PM.

Details

Summary

The lowering for v2i64 is now guarded with hasDirectMove,
however, the current lowering can handle the pattern correctly,
only lowering it when there is efficient patterns and corresponding
instructions.

The original guard was added in D21135, and was for Legal action.
The code has evloved now, this guard is not necessary anymore.

Diff Detail

Event Timeline

jsji created this revision.Jul 7 2021, 2:59 PM
jsji requested review of this revision.Jul 7 2021, 2:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 2:59 PM
qiucf added a comment.Jul 7 2021, 9:20 PM

Code change looks fine

llvm/test/CodeGen/PowerPC/aix-vec-arg-spills-mir.ll
15–16

Why are the lines indented?

llvm/test/CodeGen/PowerPC/build-vector-allones.ll
36

The operands are scrubbed. Maybe the script needs fix.

jsji added inline comments.Jul 8 2021, 7:02 AM
llvm/test/CodeGen/PowerPC/aix-vec-arg-spills-mir.ll
15–16

There are generated by script, I guess there is some slight change in script .

llvm/test/CodeGen/PowerPC/build-vector-allones.ll
36

Good catch, nothing to do with script, I manually update this, I will re-run the script.

jsji updated this revision to Diff 357217.Jul 8 2021, 7:12 AM

Update test using script.

I took a brief look at the custom lowering for BUILD_VECTOR and as far as I can tell, we correctly guard within that function (and functions it calls). So I don't think there is really an issue with removing this guard.
That being said, I'd like to see a statement about thorough testing of this patch. What I would like to see is at least a bootstrap with -mcpu=pwr7 for both LE and BE. Honestly, I don't actually know if we can successfully bootstrap on LE with -mcpu=pwr7 but if we do it would be good to test that with this patch as well. Of course, I realize that LE isn't supposed to be supported with Power7, but AFAIK it works just fine and may trigger some more issues. All we are really interested in are compiler crashes such as selection errors.

jsji added a comment.Jul 9 2021, 9:15 PM

I have tested this patch with test suite + -mcpu=pwr7 on AIX.
Sure, if you insist on bootstrap testing, I can try that too.

jsji added a comment.Jul 10 2021, 6:50 PM

@nemanjai 3 stage bootstrap ( LIT stage test in all stages, test-suite test in stage 3) with -mcpu=pwr7 -O3 + this patch passed on LE machines .

nemanjai accepted this revision.Jul 10 2021, 8:23 PM

@nemanjai 3 stage bootstrap ( LIT stage test in all stages, test-suite test in stage 3) with -mcpu=pwr7 -O3 + this patch passed on LE machines .

Awesome! Thank you so much for doing that Jinsong.
LGTM.

This revision is now accepted and ready to land.Jul 10 2021, 8:23 PM
jsji added a comment.Jul 11 2021, 8:34 PM

@nemanjai 3 stage bootstrap ( LIT stage test in all stages, test-suite test in stage 3) with -mcpu=pwr7 -O3 + this patch passed on LE machines .

Awesome! Thank you so much for doing that Jinsong.
LGTM.

Thanks. BTW, AIX bootstrap with -mcpu=pwr7 also passed.