[PATCH] D24521: [ARM] Add Marvell PJ4 cpu

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Kai,

Below are some inline reviews, but overall, it's looking good. Please update to the current trunk and check the GCC behaviour on your kits to see if they do the right thing (and what arch names they use), so we don't deviate from the norm.

thanks,
--renato



================
Comment at: include/llvm/Support/ARMTargetParser.def:227
              (ARM::AEK_HWDIVARM | ARM::AEK_HWDIVTHUMB))
+ARM_CPU_NAME("marvell-pj4", ARMV7A, FK_VFPV3, false,
+             (ARM::AEK_HWDIVARM | ARM::AEK_HWDIVTHUMB))
----------------
Move this declaration down below `Non-standard Arch names.`.


================
Comment at: include/llvm/Support/ARMTargetParser.def:228
+ARM_CPU_NAME("marvell-pj4", ARMV7A, FK_VFPV3, false,
+             (ARM::AEK_HWDIVARM | ARM::AEK_HWDIVTHUMB))
 ARM_CPU_NAME("cortex-r4", ARMV7R, FK_NONE, true, ARM::AEK_NONE)
----------------
According to the kernel link, only `PJ4B-MP / PJ4C` have `ideva`, while all have `idivt`, so I'd not add the support for `AEK_HWDIVARM` here. Maybe we should add two flavours, 'marvell-pj4' and 'marvell-pj4c'? What does GCC do?

This is the first arch that we have publicly that supports `AEK_IWMMXT2`, we should make use of that. :)


================
Comment at: lib/Support/Host.cpp:255
+        // Architecture must be ARMv7.
+        if (Lines[I].substr(16).ltrim("\t :") != "7")
+          return "generic";
----------------
This looks a bit dangerous to rely on. Basically, if this is Marvell and the CPU part below is correct, we should be good to go. I'd remove the cpu arch check altogether. See Qualcomm's implementation on recent trunk.


================
Comment at: lib/Support/Host.cpp:265
+            .Case("0x581", "marvell-pj4") // Sheeva PJ4 / PJ4B
+            .Case("0x584", "marvell-pj4") // Sheeva PJ4B-MP / PJ4C
+            .Default("generic");
----------------
Right, these guys are different in the `idiva` implementation, and maybe we should support both of them as separate.


https://reviews.llvm.org/D24521