This is an archive of the discontinued LLVM Phabricator instance.

Refactoring of how ARMTargetInfo handles default target features
ClosedPublic

Authored by labrinea on Jul 17 2015, 9:02 AM.

Details

Diff Detail

Event Timeline

labrinea updated this revision to Diff 30005.Jul 17 2015, 9:02 AM
labrinea retitled this revision from to Refactoring of how ARMTargetInfo handles default target features .
labrinea updated this object.
labrinea added inline comments.Jul 17 2015, 9:05 AM
lib/Basic/Targets.cpp
4255

StringSwitch has a limited number of arguments.

4266

Same here.

rengolin edited edge metadata.Jul 17 2015, 10:27 AM

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

Hi Renato,

Can you be a bit more specific on how to implement this? Is ArchExtKind currently used anywhere in clang?

Thanks,
Alexandros

Can you be a bit more specific on how to implement this?

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.

labrinea added a comment.EditedJul 21 2015, 6:14 AM

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.

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 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.

labrinea edited edge metadata.Jul 23 2015, 7:40 AM
labrinea added a subscriber: vsukharev.
labrinea added a comment.EditedJul 28 2015, 6:08 AM

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?
labrinea updated this revision to Diff 30821.Jul 28 2015, 8:35 AM

The reviewers have not replied yet to my last comment. Attaching the new patch now to reduce the waiting time for feedback.

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?

labrinea added inline comments.Jul 29 2015, 5:30 AM
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;
}
labrinea updated this revision to Diff 30894.Jul 29 2015, 5:31 AM
labrinea updated this object.

Patch split.

labrinea updated this revision to Diff 31045.EditedJul 30 2015, 10:28 AM

Treat "hwdiv" as architecture feature rather than extension in arm v7r, v7(e)m, v8a. (See http://reviews.llvm.org/D11590 : Diff 31043.)

Treat "hwdiv" as architecture feature rather than extension in arm v7r, v7(e)m, v8a. (See http://reviews.llvm.org/D11590 : Diff 31043.)

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.

labrinea updated this revision to Diff 31137.Jul 31 2015, 11:00 AM

Sorry for updating this before D11590 gets its final shape. But since it is probably ready for commit, and I will be away for 2-3 weeks @Richard Barton could commit this on behalf of me.

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.

rengolin added inline comments.Aug 3 2015, 2:41 AM
lib/Basic/Targets.cpp
4411

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.

labrinea updated this revision to Diff 33935.Sep 3 2015, 5:24 AM
labrinea updated this object.

Rebased

rengolin accepted this revision.Sep 4 2015, 8:04 AM
rengolin edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Sep 4 2015, 8:04 AM
This revision was automatically updated to reflect the committed changes.