This is an archive of the discontinued LLVM Phabricator instance.

Refactor ARM subarchitecture parsing.
ClosedPublic

Authored by enefaim on Jul 3 2014, 9:06 AM.

Details

Summary

According to a FIXME in ARMMCTargetDesc.cpp the ARM version parsing should be in the Triple helper class.

Diff Detail

Event Timeline

enefaim updated this revision to Diff 11057.Jul 3 2014, 9:06 AM
enefaim retitled this revision from to Refactor ARM subarchitecture parsing..
enefaim updated this object.
enefaim edited the test plan for this revision. (Show Details)
enefaim added reviewers: t.p.northover, rengolin.
enefaim added a subscriber: Unknown Object (MLST).
rengolin accepted this revision.Jul 3 2014, 9:29 AM
rengolin edited edge metadata.

Hi Gabor,

Thanks for working on this, looks a lot better! I think we have tests for all of them, and I assume they're all passing.

With the changes below, LGTM.

cheers,
--renato

include/llvm/ADT/Triple.h
81

I'd number them in sequential order, so you can do tricks like Arch > v5.

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
163

you could add the NoSubArch here, as a default.

This revision is now accepted and ready to land.Jul 3 2014, 9:29 AM
t.p.northover added inline comments.Jul 4 2014, 4:44 AM
include/llvm/ADT/Triple.h
81

I actually quite like the original: comparing architectures like that on ARM is extremely suspect since they form a partial order (Is v6m > v4t?).

Sorting in some kind of reverse (as here) reduces the temptation to do that since even the obvious comparisons will be "wrong".

84

Triple is generic code. At the very least we should prefix these with "ARMSubArch" or something to avoid polluting other targets.

rengolin added inline comments.Jul 4 2014, 4:49 AM
include/llvm/ADT/Triple.h
81

Using reverse order allows you to compare, but in the wrong order, which is even worse... :)

I agree many comparisons will be "wrong", but some are still "ok", and having them in order would make it less weird.

But this is just a weak opinion, I'm fine either way.

84

Good point!

enefaim added inline comments.Jul 4 2014, 10:34 AM
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
163

I tried to add it, but it made 12 tests fail:
Failing Tests (12):

LLVM :: CodeGen/ARM/2010-03-18-ldm-rtrn.ll
LLVM :: CodeGen/ARM/2013-04-05-Small-ByVal-Structs-PR15293.ll
LLVM :: CodeGen/ARM/2014-02-21-byval-reg-split-alignment.ll
LLVM :: CodeGen/ARM/arguments.ll
LLVM :: CodeGen/ARM/armv4.ll
LLVM :: CodeGen/ARM/call_nolink.ll
LLVM :: CodeGen/ARM/debug-frame.ll
LLVM :: CodeGen/ARM/ehabi.ll
LLVM :: CodeGen/ARM/inlineasm-ldr-pseudo.ll
LLVM :: CodeGen/ARM/integer_insertelement.ll
LLVM :: CodeGen/ARM/jump_tables.ll
LLVM :: MC/ARM/arm-thumb-cpus.s
rengolin added inline comments.Jul 4 2014, 10:48 AM
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
163

Interesting... Maybe the default sub-arch should be v4 not v4t.

enefaim added inline comments.Jul 4 2014, 11:01 AM
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
163

Yes, that works. I'm going to add 'v4' as default, and add the ARMSubArch prefixes that Tim suggested.

rengolin added inline comments.Jul 4 2014, 11:02 AM
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
163

Sounds good, thanks!

enefaim updated this revision to Diff 11093.Jul 4 2014, 11:42 AM
enefaim edited edge metadata.

Added 'ARMSubArch' prefix for the SubArchType enum.
Added 'v4' as default ARMArchFeature in the ParseArmTriple function.

Hi Gabor,

I leave the decision of adding a specific v4 to you, I don't mind much. Otherwise, LGTM.

Thanks!
--renato

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
166

I'm thinking if we need an ARMSubArch_v4 as well as the NoSubArch...

While it feels wrong to not have it, I don't think it matters much...

enefaim updated this revision to Diff 11114.Jul 7 2014, 4:16 AM

Added ARMSubArch_v4 to the SubArch enum and the ParseArmTriple function.

Renato, could you land it for me? I don't have commit rights.

Thanks,
Gabor

rengolin closed this revision.Jul 7 2014, 1:16 PM

Committed in r212479.

cheers,
--renato