Use ".arch_extension" ARM directive to specify the additional CPU features

[sgundapa]

Summary

This patch is in response to r223147 where the avaiable features are
computed based on ".cpu" directive. This will work clean for the standard
variants like cortex-a9. For custom variants which rely on standard cpu names
for aassembly, the additional features of a CPU should be propagated. This can be
done via ".arch_extension" as long as the assembler supports it.

[t.p.northover]

This looks like only half a patch (it adds a new virtual method, but nothing calls it and there are no tests). Did you accidentally send the wrong file?

[sgundapa]

Yes Tim. This patch just adds the infrastructure to add the ".arch_extension".
My next patch is to emit ".cpu" and ".arch_extension" on krait CPU and add unit tests.

[t.p.northover]

I think it'd be best to merge the two then. At the moment it's just dead code if we commit this patch as it is.

Cheers.

Tim.

[rengolin]

Hi Sumanth,

Please, add the real use to this patch, as Tim said. You'll also want to change the binary output (object files), so that it doesn't matter which output you chose, the results will be the same. That also involves changing the build attributes.

cheers,
--renato

[sgundapa]

Sure guys. I will add the use case and update an unit test.

Renote, I am not sure on what you meant by this ?

You'll also want to change the binary output (object files), so that it doesn't matter which output you chose, the results will be the same. That also involves changing the build attributes.

[rengolin]

In D7316#116989, @sgundapa wrote:

Renote, I am not sure on what you meant by this ?

You'll also want to change the binary output (object files), so that it doesn't matter which output you chose, the results will be the same. That also involves changing the build attributes.

So, in the integrated assembler, we deal with .fpu, .arch etc and change the feature bits, so that the IAS can cope with changing the default attributes. So, if you add tests, you'll have to make sure both assembly output has the new flag, but as well that the IAS works by understanding it from either internally, by emitting the correct instruction your Krait CPU can, but not A9, as well as being able to read an assembly file with A9 as CPU + arch_extension, or even "krait" as CPU, and not fail with "invalid instruction".

Does that make sense?

[echristo]

Probably want to start a new patch thread when this happens?

[sgundapa]

I am drafting an unit test on the lines of
llc < %s -mcpu=scorpion (all possible and exclusion scenarios)

Are there any pointers on creating the IAS testing ?

[rengolin]

There are two paths:

1. directly to object file:

$ llc -filetype=obj test.ll | llvm-objdump -d | FileCheck
; CHECK: whatever

2. from asm file into object:

$ llc -filetype=asm test.ll | llvm-mc -filetype=obj | llvm-objdump -d | FileCheck
; CHECK: whatever

cheers,
--renato

[sgundapa]

I have updated the patch with the support for krait hwdiv. ".arch_extension" is used to compute the feature bits for krait.
I have also updated a test case testing both the paths suggested by Renato

[rengolin]

Hi Sumanth,

The patch is looking good, thanks!

I'm just wondering if you could end up having a Krait with HDIV only in ARM and chose Thumb2. If so, than my comments need addressing.

cheers,
--renato

lib/Target/ARM/ARMAsmPrinter.cpp

682 ↗

(On Diff #19282)

shouldn't you check here based on the instruction set you're using? Something like:

if (Subtarget->isKrait() && (
    Subtarget->hasDivide() ||
    (!Subtarget->isThumb() && Subtarget->hasDivideInARMMode())
  )

To make sure we don't turn it on when it's thumb and it only has div on ARM.

test/CodeGen/ARM/krait-cpu-div-attribute.ll

11 ↗

(On Diff #19282)

If the above change is needed (checking for ISA on hdiv), then we'll need the corresponding lines here.

[echristo]

If you could upload more context in the future with your patches, they're a little light.

One comment inline.

[rengolin]

Eric, you forgot to inline your comment... :)

[echristo]

Ah. Thanks.

Here you go.

lib/Target/ARM/ARMAsmPrinter.cpp

682 ↗

(On Diff #19282)

Using subtargets here is not going to be allowed as soon as I can separate the code out so I'd prefer you come up with another way of figuring out the ISA here.

Honestly a loop over all functions in the module and collecting cpu information is probably how we're going to need to go and should be simple. I'm not sure how ARM attribute merging works between functions so if someone could detail that it would be appreciated.

[rengolin]

lib/Target/ARM/ARMAsmPrinter.cpp

682 ↗

(On Diff #19282)

ARM has no function specific attribute in EABI. The only thing we do is in the assembler just for the sake of accepting otherwise invalid instructions (bump up support) for the rare case where you're pretty sure no one will call that function without support for that instruction. This has mainly two users: ifunc and unwinders.

The point here is to work around a GAS deficiency in representing "krait" as an A9+DIV in a *global* context. So, your function specific attribute will either not exist, or be replicated to all functions, whatever is easier for you.

Regarding getting the ISA, I was wrong, and we should use the build attributes anyway.

[sgundapa]

lib/Target/ARM/ARMAsmPrinter.cpp

682 ↗

(On Diff #19282)

Guys, I am bit confused here what needs to be done.

Renato, "hasDivide()" checks for thumb2 and "hasDivideInARMMode()" checks for arm mode, the two modes where hwdiv is allowed.
Or, I am not understanding what you meant ?

ARMAsmPrinter.cpp has loads of references to Subtarget. Wouldn't it be a good idea to clean them in a single go ?

[sgundapa]

Ping !, if you want me to do anything in specific

[t.p.northover]

Just a couple of minor points:

include/llvm/MC/MCStreamer.h

142

I think this either needs a generic implementation, or (probably better for now) moving to ARMELFStreamer.cpp entirely. OK, so it's not called, but

lib/Target/ARM/ARMAsmPrinter.cpp

644 ↗

(On Diff #19282)

Any news on this, by the way? It's rather a horrific hack to have to carry, and it only seems to be getting worse.

[rengolin]

include/llvm/MC/MCStreamer.h

142

ARMELFStreamer? But this is assembly only...

lib/Target/ARM/ARMAsmPrinter.cpp

644 ↗

(On Diff #19282)

You know, I've been thinking... With the number of third-party ARM CPUs out there, maybe it would be a better model to emit .cpu as a core ARM name, and use .arch_extension to all little differences, for all other cores.

Of course, "if (Krait)" is not what I'm suggesting, but maybe a spec of each CPU somewhere, possible extracted from TableGen files, could go a long way of doing this right and transparent...

682 ↗

(On Diff #19282)

Renato, "hasDivide()" checks for thumb2 and "hasDivideInARMMode()" checks for arm mode, the two modes where hwdiv is allowed.

That's what I meant, so it should be ok.

ARMAsmPrinter.cpp has loads of references to Subtarget. Wouldn't it be a good idea to clean them in a single go ?

Yes, I don't think this is work for this patch.

[t.p.northover]

include/llvm/MC/MCStreamer.h

142

Yep, but it's essentially en ELF-dialect assembly feature so the AsmStreamer seems to live in the same file.

[sgundapa]

include/llvm/MC/MCStreamer.h

142

p

142

A quick google on ".arch_extension" suggests that this directive is specific to ARM as.
It is fair to move it to the ARMELFStreamer.cpp file at this moment

[sgundapa]

include/llvm/MC/MCStreamer.h

142

Ignore my previous comment as I thought I added the member to MCTargetStreamer.
emitArchExtension is part of ARMTargetStreamer. My code is based on below comments in MCStreamer.h
Any thing wrong here ?

/ If target foo wants to use this, it should implement 3 classes:
/ * FooTargetStreamer : public MCTargetStreamer
/ * FooTargetAsmStreamer : public FooTargetStreamer
/ * FooTargetELFStreamer : public FooTargetStreamer
/
/ FooTargetStreamer should have a pure virtual method for each directive. For
/ example, for a ".bar symbol_name" directive, it should have
/ virtual emitBar(const MCSymbol &Symbol) = 0;
/
/ The FooTargetAsmStreamer and FooTargetELFStreamer classes implement the
/ method. The assembly streamer just prints ".bar symbol_name". The object
/ streamer does whatever is needed to implement .bar in the object file.

[t.p.northover]

include/llvm/MC/MCStreamer.h

142

Oh bother, looks like I was confused by what classes lived where. I assumed that because it was in MCStreamer.h, the definition belonged to "class MCStreamer". Sorry about that, you can ignore my comment.

[sgundapa]

include/llvm/MC/MCStreamer.h

142

No problem. Your comments made me go through MCStreamer more deeper.

lib/Target/ARM/ARMAsmPrinter.cpp

644 ↗

(On Diff #19282)

to emit .cpu as a core ARM name, and use .arch_extension to all little differences, for all other cores

This is exactly what I am trying to implement here.

possible extracted from TableGen files, could go a long way of doing this right and transparent.

I second your opinion except that I have no clue on how to implement it. This is my first patch to LLVM :). I need a lot more time to go through most of the components of LLVM

[rengolin]

Tim, Eric, any more comments?

I think for what it is, the patch is good to go. The sub-target refactoring is already covered by a bug I own (sub-target description classes).

cheers,
--renato

lib/Target/ARM/ARMAsmPrinter.cpp

644 ↗

(On Diff #19282)

Sorry, I didn't mean this patch should have that refactoring, just that this technique isn't pointing us in the wrong direction. The idea is to ultimately remove all isKrait() and similar checks from the back-end, but this is not possible today.

I agree with Tim that the new code looks it's going in the wrong direction, but in the grand scheme of things, it may not. Plus, that refactoring will have to be done more thoroughly at once, from the table-gen side.

[sgundapa]

Tim and Eric. Ping!

[sgundapa]

Renato,

I will wait for a day to see if either Tim or Eric responds. If you can approve this, I can merge.

[echristo]

I'll look at this shortly.

[sgundapa]

Ping!!

[echristo]

You'll want to rebase your patch as of today.

Thanks!

-eric

[sgundapa]

The patch has been rebased.

[echristo]

It hasn't uploaded a rebased patch?

-eric

[sgundapa]

This comment was removed by sgundapa.

[sgundapa]

This comment was removed by sgundapa.

[sgundapa]

This comment was removed by sgundapa.

[sgundapa]

Rebased the patch

[echristo]

Couple of comments/questions:

a) Formatting, please run this through clang-format.
b) The attribute changes on the testcases, what's going on there?
c) If the krait always has hardware divide why both conditionals after?
d) It seems that the .cpu directive for the krait cpu is orthogonal to the arch_extension part of this patch.
e) It doesn't seem that the arch_extension stuff is wired into the object emitter? What is supposed to happen there?

-eric

[rengolin]

In D7316#125709, @echristo wrote:

b) The attribute changes on the testcases, what's going on there?

Krait is currently modelled as A15 because it's A9+Div, but it's closer to A9 than A15, and that generates codegen issues.

There is no way of telling an external assembler that the CPU is A9+Div other than adding two directives (.cpu + .arch_extension), since GAS doesn't support ".cpu krait", and I'm not sure it should, TBH, since that's the whole point of the .arch_extension directive.

c) If the krait always has hardware divide why both conditionals after?

Because of "-mattr=-hwdiv,-hwdiv-arm".

d) It seems that the .cpu directive for the krait cpu is orthogonal to the arch_extension part of this patch.

Almost... :)

.arch_extension is needed to help GAS understand that krait is A9+Div, that's why it's in the same patch. Without it, the original patch looked completely useless. This merge was actually my fault, if you feel strongly, it should be pretty safe to split the .arch_extension from the krait changes.

e) It doesn't seem that the arch_extension stuff is wired into the object emitter? What is supposed to happen there?

The object emitter already knows that Krait is A9+Div, so the Div instructions get emitted correctly. The directive is *just* for the sake of GAS allowing Divs to be emitted.

[sgundapa]

Patch rebased to tip.
Patch is ran through clang-format.

Thanks for the clean explanation Renato.

[sgundapa]

Reviewers, are you okay with this change now ?

[echristo]

Did you upload a new version and split the patch?

[sgundapa]

d) It seems that the .cpu directive for the krait cpu is orthogonal to the arch_extension part of this patch.

Almost... :)

>.arch_extension is needed to help GAS understand that krait is A9+Div, that's why it's in the same patch. Without it, the original patch looked completely useless. This merge was actually my fault, if you feel strongly, it should be pretty safe to split the .arch_extension from the krait changes

I agree the changes are orthogonal, but with out the use case scenario, the first patch doesn't make any sense.
I started with splitting the patches and was convinced by Tim and Renato to merge them.
If you insist, I will split them as long as other reviewers agree with it. Note that one of the patches will go with out a unit test.

[sgundapa]

Ahh!! I just checked my email.
I am surprised to see the comments in email conversation but not here.

I will go through emails and do the necessary.

[sgundapa]

The krait hwdiv patch is here:
http://reviews.llvm.org/D7819

[echristo]

OK, so the split should be:

Patch 1: .cpu directive changes for krait
Patch 2: adding the .arch_extension directive and using it for krait.

-eric

[rengolin]

Eric,

This is the .arch_extension implementation. Review D7819 is the Krait usage.

cheers,
--renato

[rengolin]

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

254

Can you change this to be an enum, like CPU/FPU?

[echristo]

Renato: Yes, I realize that. Did you read what I wrote?

[sgundapa]

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

254

Do you expect me to define a ".h" and ".def" files ?
If so, should I start with a single entry corresponding to hwdiv of krait ?

[sgundapa]

Hopefully, this addresses everyone's concerns

[sgundapa]

The krait usage patch is here:
http://reviews.llvm.org/D7819

[rengolin]

Excellent, that's exactly what I was talking about. :)

Some tests for this one will be in the next patch. You could hint that on the commit message.

Sorry for having taken so long. LGTM.

cheers,
--renato

include/llvm/MC/MCStreamer.h

                                     StringRef StringValue = "");
   virtual void emitFPU(unsigned FPU);
   virtual void emitArch(unsigned Arch);
+  virtual void emitArchExtension(unsigned ArchExt);
   virtual void emitObjectArch(unsigned Arch);
   virtual void finishAttributeSection();
   virtual void emitInst(uint32_t Inst, char Suffix = '\0');

lib/Target/ARM/ARMArchExtName.h

+//===-- ARMArchExtName.h - List of the ARM Extension names ------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIB_TARGET_ARM_ARMARCHEXTNAME_H
+#define LLVM_LIB_TARGET_ARM_ARMARCHEXTNAME_H
+
+namespace llvm {
+namespace ARM {
+
+enum ArchExtKind {
+  INVALID_ARCHEXT = 0
+
+#define ARM_ARCHEXT_NAME(NAME, ID) , ID
+#include "ARMArchExtName.def"
+};
+
+} // namespace ARM
+} // namespace llvm
+
+#endif

lib/Target/ARM/ARMArchExtName.def

+//===-- ARMArchExtName.def - List of the ARM Extension names ----*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains the list of the supported ARM Architecture Extension
+// names. These can be used to enable the extension through .arch_extension
+// attribute
+//
+//===----------------------------------------------------------------------===//
+
+// NOTE: NO INCLUDE GUARD DESIRED!
+
+#ifndef ARM_ARCHEXT_NAME
+#error "You must define ARM_ARCHEXT_NAME(NAME, ID) before including ARMArchExtName.h"
+#endif
+
+ARM_ARCHEXT_NAME("crc", CRC)
+ARM_ARCHEXT_NAME("crypto", CRYPTO)
+ARM_ARCHEXT_NAME("fp", FP)
+ARM_ARCHEXT_NAME("idiv", HWDIV)
+ARM_ARCHEXT_NAME("mp", MP)
+ARM_ARCHEXT_NAME("sec", SEC)
+ARM_ARCHEXT_NAME("virt", VIRT)
+
+#undef ARM_ARCHEXT_NAME

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

 
 #include "ARMArchName.h"
 #include "ARMFPUName.h"
+#include "ARMArchExtName.h"
 #include "ARMRegisterInfo.h"
 #include "ARMUnwindOpAsm.h"
 #include "llvm/ADT/StringExtras.h"

   return 0;
 }
 
+static const char *GetArchExtName(unsigned ID) {
+  switch (ID) {
+  default:
+    llvm_unreachable("Unknown ARCH Extension kind");
+    break;
+#define ARM_ARCHEXT_NAME(NAME, ID)                                             \
+  case ARM::ID:                                                                \
+    return NAME;
+#include "ARMArchExtName.def"
+  }
+  return nullptr;
+}
+
 namespace {
 
 class ARMELFStreamer;

   void emitIntTextAttribute(unsigned Attribute, unsigned IntValue,
                             StringRef StrinValue) override;
   void emitArch(unsigned Arch) override;
+  void emitArchExtension(unsigned ArchExt) override;
   void emitObjectArch(unsigned Arch) override;
   void emitFPU(unsigned FPU) override;
   void emitInst(uint32_t Inst, char Suffix = '\0') override;

 void ARMTargetAsmStreamer::emitArch(unsigned Arch) {
   OS << "\t.arch\t" << GetArchName(Arch) << "\n";
 }
+void ARMTargetAsmStreamer::emitArchExtension(unsigned ArchExt) {
+  OS << "\t.arch_extension\t" << GetArchExtName(ArchExt) << "\n";
+}
 void ARMTargetAsmStreamer::emitObjectArch(unsigned Arch) {
   OS << "\t.object_arch\t" << GetArchName(Arch) << '\n';
 }

lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp

                                              unsigned IntValue,
                                              StringRef StringValue) {}
 void ARMTargetStreamer::emitArch(unsigned Arch) {}
+void ARMTargetStreamer::emitArchExtension(unsigned ArchExt) {}
 void ARMTargetStreamer::emitObjectArch(unsigned Arch) {}
 void ARMTargetStreamer::emitFPU(unsigned FPU) {}
 void ARMTargetStreamer::finishAttributeSection() {}