- Used TargetParser to get default fpu features.
- Used TargetParser to get default extension features.
- Added logic to handle +fp16.
- Added testcases
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
Hi Alexandros,
Would it be possible to change ArchExtKind to be a bitmap, so that we can add a new column to CPUNames "unsigned DefaultExt;" and & all supported extensions?
That way, all your string parsing routines in Clang will be converted to a table lookup in LLVM, making our lives easier when we do move it to table gen.
You may encounter problems on build attributes, though.
cheers,
--renato
Comment Actions
Hi Renato,
Can you be a bit more specific on how to implement this? Is ArchExtKind currently used anywhere in clang?
Thanks,
Alexandros
Comment Actions
If you look at ARMAsmParser.cpp, you'll find an extension table just before parseDirectiveArchExtension.
That table is a mapping between the parser's own enum to Table-gen's features. Since the mapping is not one-to-one, we need to keep the parser's IDs, and since the parser is still not table-generated, we can't have those feature bits in the TargetParser class.
An interim alternative would be to move the features in TargetParser to either a bitfield or an enum setting individual bits (0x1, 0x2, 0x4, etc). Then, you'd have to change the checks to be (Kind & K) instead of (Kind == K) and then you could add the default flags to the table:
struct {
const char *Name;
ARM::ArchKind ArchID;
ARM::FPUKind DefaultFPU;
bool Default; // is $Name the default CPU for $ArchID ?
unsigned DefaultExtensions;
} CPUNames[] = {
{ "cortex-a53", ARM::AK_ARMV8, ARM::FK_ARM::FK_CRYPTO_NEON_FP_ARMV8, true, { ARM::EK_CRYPTO | ARM::EK_CRC } },
...
};You'd have to change both LLVM and Clang sides to cope with it being a bitfield, but that would also reduce your Clang methods to something like:
void getDefaultFeatures(Features) {
unsigned DefaultFeatures = getDefaultFeatures(CPUID);
foreach (A: ARCHExtNames)
if (DefaultFeatures & A.ID)
Features[A.Name] = true;
}Is ArchExtKind currently used anywhere in clang?
Not yet. I was talking about the ones you are introducing with this patch.
Comment Actions
Ok Renato I see what you mean. It's getting a bit complicated since it's strongly connected to my upcoming patch for ACLE 2.0 macros. I think we have to decide what should be present in ArchExtKind. For example:
- FP is not 1-bit value, can be SP | HP | DP. Furthermore I can't see how is this value useful since FP support it is currently handled by checking the (v)fp* features that ARMTargetParser::getFPUFeatures pushes.
- HWDIV is not 1-bit value, can be ARM | THUMB.
- CRYPTO is already handled by ARMTargetParser::getFPUFeatures, maybe it should not be in ArchExtKind?
More over should the following be present in ArchExtKind?
- ACLE 6.4.7 DSP instructions.
- ACLE 6.4.8 Saturation instructions.
- ACLE 6.4.9 32-bit SIMD instruction (I guess AEK_SIMD refers to that).
What about the rest? What do they represent? Is it planned to be handled somewhere in clang's logic?
- AEK_MP
- AEK_SEC
- AEK_VIRT
Finally some of these features are not handled by the back-end that's why we might need to remove after being handled by the front-end.
Comment Actions
I agree, this is very confusing. What ArchExt is trying to encode is what can be passed down via extension flags in mcpu, for instance "cortex-a53+crc".
We still need the parser, the accepted values and a list of enums to represent them, but we should try to common up things as close as possible to what is in the *.td files.
I think we have to decide what should be present in ArchExtKind. For example:
Whatever is there needs to remain there, as it is, or target extension parameters won't work. But we can add new methods to ARMTargetParser to encode the extra logic we need, by creating relationship tables and accessors.
- FP is not 1-bit value, can be SP | HP | DP. Furthermore I can't see how is this value useful since FP support it is currently handled by checking the (v)fp* features that ARMTargetParser::getFPUFeatures pushes.
- CRYPTO is already handled by ARMTargetParser::getFPUFeatures, maybe it should not be in ArchExtKind?
These are used for the sole purpose of setting the right flags on .arch_extension directives in the ARM assembler for ARMv8. And the table that does that is *not* in TargetParser, but AsmParser, which is the wrong place. But since TargetParser has no access to table-gen, it has to be there for the time being.
Moving that one out is a whole patch in its own.
Also, FPU has some accessory tables (FPUVersion, NeonSupportLevel, etc). We could have the same for the other multiple-choice features.
- HWDIV is not 1-bit value, can be ARM | THUMB.
Level of support is based on CPU, and that's why I suggested having those flags in the CPU table. An alternative would be to create an enum for CPUs (like we have for Arch and VFP), but I don't like this idea, since CPU names are not unique.
More over should the following be present in ArchExtKind?
Whatever we are willing to support in -mcpu / .arch_extension directives.
Finally some of these features are not handled by the back-end that's why we might need to remove after being handled by the front-end.
The whole point of the TargetTuple, TargetParser and all is to make sure both front- and back-end talk the same language. If the back-end doesn't support a feature, it should at least understand, and in the same way the front-end does.
We may need to change the features in ARM.td a bit. We will need to move all table lookups to TargetParser. And we well need to generate the TargetParser tables with table-gen.
In the end, methods like:
isARMHDiv(cpuname);
should be in the TargetParser itself.
Comment Actions
I have a new patch almost ready for this thread and some questions about it:
- Since it will be split in two (clang, llvm) and llvm has to go first there will be regressions in clang because the tests will be outdated until the second patch is committed. How should I deal with this?
- What happens with hwdiv, hwdiv-arm, idiv? Clang currently ignores idiv: " '+idiv' is not a recognized feature for this target (ignoring feature) ". Maybe bypass idiv when iterating through the arc extensions and exclude it from StringMap<bool> Features, with a FIXME label on it?
Comment Actions
The reviewers have not replied yet to my last comment. Attaching the new patch now to reduce the waiting time for feedback.
Comment Actions
Hi Alexandros,
The patch is getting a good shape. Please rebase and split in two separate reviews, one for LLVM and one for Clang.
The idiv comment just needs to be checked that, when choosing the cores with only one HWDIV option, idiv is correctly selected. If not, we need to change the emission and add tests on the LLVM side.
cheers,
--renato
| lib/Support/TargetParser.cpp | ||
|---|---|---|
| 217 ↗ | (On Diff #30821) | I worry how this is going to affect the emission of idiv in assembler (ELFStreamer). Maybe we need to emit "idiv" on something like: if (getDefaultExtensions(CPU) & (HWDIV | HWDIVARM)) ... |
| 225 ↗ | (On Diff #30821) | @richard.barton.arm Can you have a look at these defaults to make sure they're all correct? |
| lib/Support/TargetParser.cpp | ||
|---|---|---|
| 217 ↗ | (On Diff #30821) | I am not sure I understood this comment. I wouldn't want to do any changes in assembler, at least for this patch. I initially intended to refactor how ARMTargetInfo handles default target features, and thus I believe changing ELFStreamer is out of the scope of this patch. What I could do is to provide API in TargetParser if you find it useful: bool ARMTargetParser::hasExtension(StringRef CPU, StringRef extension) {
for (const auto AE : ARCHExtNames) {
if (extension == AE.Name) {
unsigned DefaultExtensions = getDefaultExtensions(CPU);
return (DefaultExtensions & AE.ID);
}
}
return false;
} |
Comment Actions
Treat "hwdiv" as architecture feature rather than extension in arm v7r, v7(e)m, v8a. (See http://reviews.llvm.org/D11590 : Diff 31043.)
Comment Actions
The LLVM side of it is a bit controversial, let's keep this patch in brine for a while, until we figure out the other side.
Comment Actions
I haven't looked at this one in detail yet, and I'm waiting until we agree on the LLVM part before I can start looking at it.
| lib/Basic/Targets.cpp | ||
|---|---|---|
| 4465 ↗ | (On Diff #31137) | If we don't support "hwdiv" and "hwdiv-arm" in the TargetParser (because that's a Clang thing, not an ARM thing), we can easily set both when either of them are default on the CPU and unset hwdiv-arm for M class. At least until we have a better way of separating Clang specific strings from ARM generic ones. The point of the target parser is NOT to become a "many-things-parser", but just target specific strings that are valid *everywhere*. Clang-specific logic / strings should remain in Clang. |