[PATCH] D28291: [ARM] Create SubtargetFeatures from build attributes

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: test/tools/llvm-objdump/ARM/v7a-subfeature.s:1
+@ RUN: llvm-mc < %s -triple armv7a -mattr=+vfp3,+neon,+fp16,+hwdiv-arm -filetype=obj | llvm-objdump -triple=arm -d - | FileCheck %s
+
----------------
samparker wrote:
> rengolin wrote:
> > samparker wrote:
> > > rengolin wrote:
> > > > If you pass the `-mattr`, it doesn't need to read the `.eabi_attributes`, so what does this test really test?
> > > As far as I can tell, the mattr is used by mc to detect valid instructions, but it is not used to encode the build attributes. So they're needed in the test so the attributes are encoded for objdump to read.
> > That's my point. Here you have two ways to setup the attributes: `-marttr` and `.eabi_attribute`. What you need is to make sure that *both* ways work for your purposes.
> > 
> > If you create one test with `-mattr` and *another* with the same `.eabi_attribute`, making sure they're complete to describe what you need, than you covered both cases.
> But this patch isn't about enabling llvm-mc to write attributes that are provided via the -mattr option. These tests surely only need to prove that they can use what is provided in the binary.
Then I don't understand what the patch is about. ;)

What I see is an patch that reads the build attributes (not the -mattr) and adds those features to the target's list.

If you add the same attributes via -mattr, then reading the attributes will make no difference, ie. the test will pass without the build attributes being present. 

To make sure the build attributes parsing and feature-setting behaviour works, you should *not* have those attributes already there (via -mattr) and let the build attributes be parsed, add the features and see if it works.

what am I missing here?


https://reviews.llvm.org/D28291