This is an archive of the discontinued LLVM Phabricator instance.

Add support for parsing HWDiv features using target parser
ClosedPublic

Authored by labrinea on Jul 23 2015, 4:53 AM.

Details

Summary

*To be tested by clang.

Diff Detail

Event Timeline

labrinea updated this revision to Diff 30472.Jul 23 2015, 4:53 AM
labrinea retitled this revision from to Add support for parsing HWDiv features using target parser.
labrinea updated this object.
rengolin edited edge metadata.Jul 23 2015, 5:19 AM

There are two changes here: parse hwdiv and default hdiv.

The former looks good, the latter, not so much.

Please separate the patch in two and re-submit here only the parsing.

The default hwdiv will have to be implemented together with the other default flags, as we discussed earlier.

lib/Support/TargetParser.cpp
379

oops

labrinea edited edge metadata.Jul 24 2015, 3:34 AM
labrinea added subscribers: vsukharev, javed.absar.
labrinea updated this revision to Diff 30631.Jul 24 2015, 8:02 PM

So, overall looks ok.

AFAICT, these values are never used as is, to use in build attributes or somewhere else, so it should be safe to change their values to what we want.

Right now, the only uses of these are either to parse (string->ID) in Clang and the assembler and ID->string in ARMAsmPrinter.cpp, or to map them to build attribute flags in ARMAsmParser.cpp.

I don't know what default value you'd put on "idiv", but I'm guessing AEK_HWDIV would be the best choice.

cheers,
--renato

include/llvm/Support/TargetParser.h
137–141

I'd put these ones at the end of the 32-bit range.

142

you don't need AEK_LAST

lib/Support/TargetParser.cpp
124

you can't change this. This is the string that is supported by the command line and the assembler.

The extensions supported by -mcpu here:

https://sourceware.org/binutils/docs/as/ARM-Options.html

are the same supported by the .arch_extension assembler directive.

If this code passed the Clang tests, it means there aren't enough tests in there for these flags.

labrinea updated this revision to Diff 30685.Jul 27 2015, 5:05 AM

changed according to reviewer comments

I don't know what default value you'd put on "idiv", but I'm guessing AEK_HWDIV would be the best choice.

ACLE 2.0 says __ARM_FEATURE_IDIV should be defined when both arm and thumb divide are supported. That's why I set the default value to (ARM::AEK_HWDIVARM | ARM::AEK_HWDIV)

rengolin added inline comments.Jul 27 2015, 5:11 AM
lib/Support/TargetParser.cpp
123

Let's keep these two out of it.

This is a compatibility issue and we don't want to create more unnecessary divergence than we already have.

This string will be parsed as "-mcpu=cpuname+idiv", but we already have the alternative (non-compatible) "-mhwdiv=arm/thumb", which already does what we need.

289

Couldn't you just do something like:

if (HWDivKind & ARM::AEK_HWDIV)
  Features.push_back("+hwdiv");
else
  Features.push_back("-hwdiv");

if (HWDivKind & ARM::AEK_HWDIVARM)
  Features.push_back("+hwdiv-arm");
else
  Features.push_back("-hwdiv-arm");

ACLE 2.0 says __ARM_FEATURE_IDIV should be defined when both arm and thumb divide are supported. That's why I set the default value to (ARM::AEK_HWDIVARM | ARM::AEK_HWDIV)

Perfect.

labrinea added inline comments.Jul 27 2015, 5:28 AM
lib/Support/TargetParser.cpp
123

The idea here was to be able to do what you suggested before http://reviews.llvm.org/D11299:

void getDefaultFeatures(Features) {

unsigned DefaultFeatures = getDefaultFeatures(CPUID);
foreach (A: ARCHExtNames)
  if (DefaultFeatures & A.ID)
    Features[A.Name] = true;

}

Clang knows how to handle "+hwdiv-arm" and "+hwdiv" but not "+idiv" (at least in ARMTargetInfo).

289

Sure.

417

ARM::AEK_LAST no longer exists. I have to change this function to be iterative. Also add an equivalent function for getting HWDivName

rengolin added inline comments.Jul 27 2015, 5:41 AM
lib/Support/TargetParser.cpp
123

That's different. +hwdiv ends up as a target-feature, and we're not parsing them.

Besides, you're not using them in this patch, so it should not be added here, now.

labrinea updated this revision to Diff 30687.Jul 27 2015, 6:34 AM

Changed according to reviewer comments.

rengolin accepted this revision.Jul 27 2015, 6:50 AM
rengolin edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Jul 27 2015, 6:50 AM
labrinea added inline comments.Jul 27 2015, 8:27 AM
lib/Support/TargetParser.cpp
123

I did not commit yet. Setting the ID to (ARM::AEK_HWDIVARM | ARM::AEK_HWDIV) causes regressions in llvm/tests. I think the reason is that in file lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp, getArchExtName() is called with ARM::AEK_HWDIV as argument, table lookup returns invalid and the function returns a null string. This is just one regression (CodeGen/ARM/krait-cpu-div-attribute.ll). There are more tests failing that I have not investigated yet. Shall I set ID to ARM::AEK_HWDIV as it was initially?

rengolin added inline comments.Jul 27 2015, 8:45 AM
lib/Support/TargetParser.cpp
123

I think you can safely change ARMELFStreamer to emitArchExtension(HWDIV | HWDIVARM), since that's what it means.

labrinea updated this revision to Diff 30701.Jul 27 2015, 9:27 AM
labrinea edited edge metadata.

Fixed regressions for ARM::AEK_HWDIV in ARMELFStreamer

Looks good, thanks!

rengolin closed this revision.Jul 28 2015, 4:14 AM

r243335