This is an archive of the discontinued LLVM Phabricator instance.

[XRay] Support AArch64 in Clang
ClosedPublic

Authored by rSerge on Nov 8 2016, 12:31 PM.

Details

Diff Detail

Event Timeline

rSerge updated this revision to Diff 77239.Nov 8 2016, 12:31 PM
rSerge retitled this revision from to [XRay] Support AArch64 in Clang.
rSerge updated this object.
rSerge added reviewers: dberris, rengolin.
rSerge added subscribers: iid_iunknown, cfe-commits.
rSerge updated this object.Nov 8 2016, 12:32 PM
dberris added inline comments.Nov 8 2016, 3:08 PM
lib/Driver/Tools.cpp
4902–4910

I'm wondering whether it's worth turning this into a switch statement now that we have more than two supported architectures?

dberris requested changes to this revision.Nov 8 2016, 7:04 PM
dberris edited edge metadata.
This revision now requires changes to proceed.Nov 8 2016, 7:04 PM
rSerge added inline comments.Nov 9 2016, 2:02 PM
lib/Driver/Tools.cpp
4902–4910

I think that would lead to more awkward code: there wouldn't be a single decision outcome point (like the current else block), so to avoid duplicating the code which currently prints the message, a bool variable would be needed. I think it's more neat to just enumerate all the OS&CPU combinations in the if condition for now.
This place is not performance-critical and the compiler should convert appropriate ifs into switches anyway.

dberris added inline comments.Nov 9 2016, 3:07 PM
lib/Driver/Tools.cpp
4902–4910

This is an issue of making it more readable, something like:

if (Triple.getOS() != llvm::Triple::Linux)
  D.Diag(...) << ...; // Unsupported OS.
switch (Triple.getArch()) {
  case llvm::Triple::arm:
  case llvm::Triple::x86_64:
  case llvm::Tripe::aarch64:
    // Supported.
    break;
  default:
    D.Diag(...) << ...;
}

This way any new architectures become supported, they just get added to the list of cases that short-circuit.

rSerge added inline comments.Nov 10 2016, 5:47 AM
lib/Driver/Tools.cpp
4902–4910

We can't say that an OS is supported or unsupported unless all CPU architectures for this OS support or don't support XRay, and this is not going to happen in the near future. So it is more accurate to say about the triple: some triples are supported and some are not. So in coding it is natural to check for the triple with || and &&.

dberris added inline comments.Nov 10 2016, 5:59 AM
lib/Driver/Tools.cpp
4902–4910

We can't say that an OS is supported or unsupported unless all CPU architectures for this OS support or don't support XRay, and this is not going to happen in the near future.

I don't get it. Right now, as written, it doesn't matter what OS it is -- any OS other than Linux wouldn't be supported anyway. Maybe I mis-wrote, but:

if (Triple.getOS() != llvm::Triple::Linux)
  D.Diag(...) << ...;
else switch(Triple.getArch()) {
  ...
  default:
    D.Diag(...) << ...;
}

Is a direct translation that's more readable than the current complex if statement conditional.

So it is more accurate to say about the triple: some triples are supported and some are not. So in coding it is natural to check for the triple with || and &&.

Sure, but conditional is already unwieldy with just three supported platforms.

rSerge updated this revision to Diff 77537.Nov 10 2016, 12:46 PM
rSerge edited edge metadata.
rSerge marked 5 inline comments as done.
rSerge added inline comments.
lib/Driver/Tools.cpp
4902–4910

Changed.

dberris added inline comments.Nov 10 2016, 3:46 PM
lib/Driver/Tools.cpp
4903–4906

Did you need to put an else before the switch? I don't know what happens when you're on Windows non-supported platform -- will this cause two diagnostics to be printed?

rSerge marked an inline comment as done.Nov 11 2016, 5:58 AM
rSerge added inline comments.
lib/Driver/Tools.cpp
4903–4906

I thought that the statement after D.Diag(diag::err_drv_clang_unsupported) will not be executed. Let me check...

rSerge updated this revision to Diff 77675.Nov 11 2016, 2:45 PM
rSerge edited edge metadata.
rSerge added inline comments.Nov 11 2016, 2:50 PM
lib/Driver/Tools.cpp
4903–4906

Indeed, it was printing 2 diagnostics. I've added else.

rSerge marked 2 inline comments as done.Nov 11 2016, 2:50 PM
dberris accepted this revision.Nov 13 2016, 5:08 PM
dberris edited edge metadata.
This revision is now accepted and ready to land.Nov 13 2016, 5:08 PM
rSerge updated this revision to Diff 78216.Nov 16 2016, 10:31 AM
rSerge edited edge metadata.

Rebased to the latest version of LLVM sources.

This revision was automatically updated to reflect the committed changes.