Page MenuHomePhabricator

Fix the prefix for arm64 triple
ClosedPublic

Authored by kongyi on Jul 15 2014, 3:38 AM.

Details

Summary

Triple.cpp still returns "arm64" as prefix for arm64 triple, causing Clang not being able to select the correct GCCBuiltin IR.

This patch changes the value to correct prefix "aarch64". Regression test will be added in the coming patch.

Diff Detail

Event Timeline

kongyi updated this revision to Diff 11433.Jul 15 2014, 3:38 AM
kongyi retitled this revision from to Fix the prefix for arm64 triple.
kongyi updated this object.
kongyi edited the test plan for this revision. (Show Details)
kongyi set the repository for this revision to rL LLVM.
kongyi added a project: deleted.
kongyi added a subscriber: Unknown Object (MLST).
rengolin edited edge metadata.Jul 15 2014, 3:44 AM

Ouch, that looks like an oversight. Tim?

t.p.northover edited edge metadata.Jul 15 2014, 8:02 AM

It looks like it's only used by that part of Clang. We probably want a test though (even if it's in Clang rather than LLVM), since this is clearly causing problems for someone.

Also, are you planning to do the more thorough triple merge, Yi?

Cheers.

Tim.

It looks like it's only used by that part of Clang. We probably want a test though (even if it's in Clang rather than LLVM), since this is clearly causing problems for someone.

I have written a test case in Clang, however due to the limitation of Phabricator, I can't post it in the same post. I will take another look to see whether it is possible to test in LLVM.

Also, are you planning to do the more thorough triple merge, Yi?

Although the merge should be trivial, but I think it would be best for you guys to do it to avoid breaking your internal Apple clients.

In D4516#9, @kongyi wrote:

It looks like it's only used by that part of Clang. We probably want a test though (even if it's in Clang rather than LLVM), since this is clearly causing problems for someone.

I have written a test case in Clang, however due to the limitation of Phabricator, I can't post it in the same post.

Actually, it's part of D4520. The test makes sure that GCCBuiltin works when the triple supplied is ARM64, indicating LLVM returned the correct prefix.

I will take another look to see whether it is possible to test in LLVM.

I can't find a way to do that. It seems we can only test this indirectly from Clang.

I have written a test case in Clang, however due to the limitation of Phabricator, I can't post it in the same post. I will take another look to see whether it is possible to test in LLVM.

No need to do that. I suspect it's very strongly linked to the C++
front-end so I'd be quite happy with a Clang test, but we should have
one.

Cheers.

Tim.

No need to do that. I suspect it's very strongly linked to the C++
front-end so I'd be quite happy with a Clang test, but we should have
one.

My plan to implement this is by D4521, where GCCBuiltin __builtin_arm_[dmb, dsb, isb] are tested to be correctly selected by Clang when arm64 triple is supplied. It's an indirect test, but that's the only test I can think of.

Do you have any suggestion? Otherwise ok to commit?

t.p.northover accepted this revision.Jul 17 2014, 2:32 AM
t.p.northover edited edge metadata.

Hi Yi,

My plan to implement this is by D4521, where GCCBuiltin __builtin_arm_[dmb, dsb, isb] are tested to be correctly selected by Clang when arm64 triple is supplied.

OK, I see. There aren't actually any AArch64 builtins yet so it can't be tested. That sounds reasonable. Go for it!

Tim.

This revision is now accepted and ready to land.Jul 17 2014, 2:32 AM
kongyi closed this revision.Jul 17 2014, 2:52 AM
kongyi updated this revision to Diff 11562.

Closed by commit rL213240 (authored by @kongyi).