[PATCH] D26415: [XRay] Support AArch64 in Clang

[Serge Rogatch via cfe-commits]

rSerge marked 5 inline comments as done.
rSerge added inline comments.


================
Comment at: lib/Driver/Tools.cpp:4903-4906
     if (Triple.getOS() == llvm::Triple::Linux &&
         (Triple.getArch() == llvm::Triple::arm ||
-         Triple.getArch() == llvm::Triple::x86_64)) {
+         Triple.getArch() == llvm::Triple::x86_64 ||
+         Triple.getArch() == llvm::Triple::aarch64)) {
----------------
dberris wrote:
> rSerge wrote:
> > dberris wrote:
> > > rSerge wrote:
> > > > dberris wrote:
> > > > > I'm wondering whether it's worth turning this into a `switch` statement now that we have more than two supported architectures?
> > > > 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 `if`s into `switch`es anyway.
> > > 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.
> > 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 `&&`.
> > 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.
Changed.


https://reviews.llvm.org/D26415