Page MenuHomePhabricator

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

Authored by sgundapa on Jan 31 2015, 12:26 PM.

Details

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.

Diff Detail

Event Timeline

sgundapa updated this revision to Diff 19098.Jan 31 2015, 12:26 PM
sgundapa retitled this revision from to Use ".arch_extension" ARM directive to specify the additional CPU features.
sgundapa updated this object.
sgundapa edited the test plan for this revision. (Show Details)
sgundapa added a subscriber: Unknown Object (MLST).

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?

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.

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 edited edge metadata.Feb 1 2015, 12:00 PM

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

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.

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?

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

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 ?

There are two paths:

  1. directly to object file:

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

  1. from asm file into object:

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

cheers,
--renato

sgundapa updated this revision to Diff 19282.Feb 3 2015, 4:13 PM
sgundapa updated this object.
sgundapa edited edge metadata.

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

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
688

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
12

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

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

One comment inline.

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

Ah. Thanks.

Here you go.

lib/Target/ARM/ARMAsmPrinter.cpp
688

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 added inline comments.Feb 4 2015, 10:54 AM
lib/Target/ARM/ARMAsmPrinter.cpp
688

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 added inline comments.Feb 4 2015, 1:53 PM
lib/Target/ARM/ARMAsmPrinter.cpp
688

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 ?

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

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

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 added inline comments.Feb 6 2015, 3:57 AM
include/llvm/MC/MCStreamer.h
142

ARMELFStreamer? But this is assembly only...

lib/Target/ARM/ARMAsmPrinter.cpp
644

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

688

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 added inline comments.Feb 6 2015, 7:13 AM
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 added inline comments.Feb 6 2015, 2:09 PM
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 added inline comments.Feb 6 2015, 2:46 PM
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 added inline comments.Feb 6 2015, 9:07 PM
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 added inline comments.Feb 8 2015, 9:15 AM
include/llvm/MC/MCStreamer.h
142

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

lib/Target/ARM/ARMAsmPrinter.cpp
644
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

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

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.

Tim and Eric. Ping!

Renato,

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

I'll look at this shortly.

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

Thanks!

-eric

sgundapa updated this revision to Diff 20124.Feb 17 2015, 4:35 PM

The patch has been rebased.

It hasn't uploaded a rebased patch?

-eric

sgundapa updated this revision to Diff 20140.Feb 17 2015, 8:32 PM
This comment was removed by sgundapa.
This comment was removed by sgundapa.
This comment was removed by sgundapa.
sgundapa updated this revision to Diff 20141.Feb 17 2015, 9:14 PM

Rebased the patch

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

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 updated this revision to Diff 20231.Feb 18 2015, 3:49 PM

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

Thanks for the clean explanation Renato.

Reviewers, are you okay with this change now ?

Did you upload a new version and split the patch?

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.

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 updated this revision to Diff 20481.Feb 22 2015, 12:34 PM
sgundapa retitled this revision from Use ".arch_extension" ARM directive to specify the additional CPU features to Use ".arch_extension" ARM directive to specify the additional CPU features.
sgundapa updated this object.

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

Eric,

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

cheers,
--renato

rengolin added inline comments.Feb 24 2015, 1:46 PM
lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
254

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

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

sgundapa added inline comments.Feb 24 2015, 6:49 PM
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 updated this revision to Diff 20716.Feb 25 2015, 4:36 PM

Hopefully, this addresses everyone's concerns

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

rengolin accepted this revision.Feb 26 2015, 8:12 AM
rengolin edited edge metadata.

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

This revision is now accepted and ready to land.Feb 26 2015, 8:12 AM
sgundapa closed this revision.Feb 27 2015, 8:52 AM