[PATCH] D69103: Backend for NEC SX-Aurora

Mon Oct 28 05:42:22 PDT 2019

simoll planned changes to this revision.
simoll marked 3 inline comments as done.
simoll added a comment.

In D69103#1722713 <https://reviews.llvm.org/D69103#1722713>, @arsenm wrote:

> Could use some unit tests in TripleTest

Ok. Skimming through the tests there, it seems one parser test for a `ve-X-Y` triple should be enough, right?

================
Comment at: llvm/lib/Support/Triple.cpp:1474
   case Triple::renderscript64:
+  case Triple::ve:

----------------
craig.topper wrote:
> It looks like this list was supposed to be alphabetized, but then renderscript32/64 was added out of order? Should we just put this back in order?
I guess the order of backend names is not a critical issue. I'd bring the target lists back into alphabetical order in a quick follow-up after this patch.

================
Comment at: llvm/lib/Target/LLVMBuild.txt:39
  XCore
+ VE

----------------
arsenm wrote:
> Targets usually start out as experimental targets, not added to the default build list (although IMO this is more trouble than it's worth in practice)
It is my understanding that a target is experimental, if it is not added by default to `LLVM_TARGETS_TO_BUILD` (it is not in `LLVM_ALL_TARGETS` of `llvm/CMakeLists.txt).

If this line is removed the cmake configure step crashes when the target is added to `LLVM_EXPERIMENTAL_TARGETS_TO_BUILD`.

    llvm-build: error: invalid target to enable: 'VE' (not in project)

================
Comment at: llvm/lib/Target/VE/MCTargetDesc/VEMCTargetDesc.h:16-18
+#include "llvm/Support/DataTypes.h"
+
+#include <memory>
----------------
arsenm wrote:
> Unnecessary includes? Although I guess this is just a stub
Yep. You are looking at a close-to-minimal slice of the current backend source on github. Should i strip these includes? (to add them later?)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69103/new/

https://reviews.llvm.org/D69103