Page MenuHomePhabricator

[ARM64]Fix the bug cannot select UQSHL/SQSHL with constant i64 shift amount.
Needs ReviewPublic

Authored by HaoLiu on Apr 24 2014, 10:57 PM.

Details

Reviewers
t.p.northover
Summary

Hi Tim and other reviewers,

Those are 2 patches for LLVM and Clang to fix UQSHL/SQSHL by constant can not be selected. The simply intrinsic call:

vqshld_s64(a, 36)  // a is a int64_t

will cause build failure.

The root cause is that 2 different pattern match types are used for the following 2 intrinsics:

int64_t vqshld_n_s64(int64_t a, const int n)      SQSHL Dd,Dn,#n
int64_t vqshld_s64(int64_t a, int64_t b)          SQSHL Dd,Dn,Dm

vqshld_n_s64 uses int64x1_t for arguments and return value. vqshld_s64 uses int64_t. If the second argument of vqshld_s64 is a constant, it will also try to generate a SQSHL by contant instrution, i.e the first kind of instruction "SQSHL Dd,Dn,#n", not "SQSHL Dd,Dn,Dm". As there is only a pattern for v1i64, not i64, there is a cannot select failure.

To fix this, we can simply add a pattern about i64 to match SQSHL by constant (The LLVM patch). But except that, I also think it is not reasonable to use int64x1_t for "int64_t vqshld_n_s64(int64_t a, const int n)", as it is originally int64_t, why we transfer int64_t to int64xt_t to match. So I also modify the Clang to use int64_t/uint64_t instead of transfering to int64x1_t/uint64x1_t (The Clang patch).

I only add two test cases about vqshld_s64 and vqshld_u64 with constant shift amount, as there are already other test cases for other situations.

Review please.
Thanks

Diff Detail

Event Timeline

HaoLiu updated this revision to Diff 8829.Apr 24 2014, 10:57 PM
HaoLiu retitled this revision from to [ARM64]Fix the bug cannot select UQSHL/SQSHL with constant i64 shift amount..
HaoLiu updated this object.
HaoLiu edited the test plan for this revision. (Show Details)
HaoLiu added a reviewer: t.p.northover.
HaoLiu added subscribers: Unknown Object (MLST), Unknown Object (MLST).
t.p.northover edited edge metadata.Apr 25 2014, 2:19 AM

Hi Hao,

The changes look reasonable, but there should be an LLVM test in test/CodeGen/ARM64 too.

Cheers.

Tim.

Hi Tim,

After added LLVM CodeGen test, these patches have been committed in http://llvm.org/viewvc/llvm-project?rev=207399&view=rev and and http://llvm.org/viewvc/llvm-project?rev=207401&view=rev.

Thanks,
-Hao