Page MenuHomePhabricator

Fix for PR20104. Enable GAS compliant coprocessor register name.
ClosedPublic

Authored by akuharev on Jun 23 2014, 3:53 AM.

Details

Summary

Added parsing for parameter names starting with "cr"

Additional compliant GAS names for coprocessor register name
are enabled for all instruction with parameter MCK_CoprocReg:
LDC,LDC2,STC,STC2,CDP,CDP2,MCR,MCR2,MCRR,MCRR2,MRC,MRC2,MRRC,MRRC2
...

Diff Detail

Event Timeline

akuharev updated this revision to Diff 10742.Jun 23 2014, 3:53 AM
akuharev retitled this revision from to Fix for PR20104. Enable GAS compliant coprocessor register name..
akuharev updated this object.
akuharev edited the test plan for this revision. (Show Details)
akuharev added a subscriber: Unknown Object (MLST).

Hi Andrey,
Thanks for working on it!
I think we have to keep compatibility with GAS.. So in general patch looks reasonable. May be it would be good to add some details what method does (see inline comments).
I've CCed Renato and Tim for approval.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
3085–3092

I think, it would be good to explain the grammar in short, something like:
RegName ::= <Prefix><Number>
Number - integer in range [0, 16)
If CoprocOp is 'c', then:
Prefix ::= c|cr
If CoprocOp is 'c', then:
Prefix ::= p

3099

Please place comment above "if" statement:
// For gas compatibility
if ...

Hi Andrey,

Addressing both mine and Stepan's comments, the patch looks good to me.

cheers,
--renato

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
3099

Since we use CoprocOp for the first char, I'd do in a similar way for the GNU compatibility.

Alternatively, assuming the name will always come lower case, and since you already tested for the first char, I'd just test:

if (Name[1] == 'r') {
  Name = Name.drop_front();
  Name[0] = CoprocOp;
}

Why the second line? Because from this point onwards, the string will be 'rN' rather than 'cN', which is why you moved the CoprocOp test upwards, but that is not future proof, and anyone writing checks below that line will get confused, or produce confusing code, such as:

if (Name[0] == 'r') ...

which doesn't help future developers.

akuharev updated this revision to Diff 10789.Jun 24 2014, 8:30 AM
akuharev added reviewers: dyatkovskiy, rengolin.

Added comments and cut prefix in switch

Hi Renato,

StringRef is wrapper under constant string and it's not acceptable:
Name[0] = CoprocOp;

It may be better to cut off prefix and parse only numbers in switch.

-Andrey

rengolin edited edge metadata.Jun 24 2014, 8:37 AM

Indeed, much better! LGTM.

Thanks,
--renato

Thanks for reviews!
I don't have commit access. Please commit the patch.

dyatkovskiy edited edge metadata.Jun 25 2014, 10:38 PM

Yup, Renato proposed good idea. Final patch looks fine, except tiny thing, could you use single drop_front call (see inline comments)?

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
3101

May be like this?
Name = (Name[1] == 'r') ? Name.drop_front(2) : Name.drop_front();

akuharev updated this revision to Diff 10880.Jun 26 2014, 12:51 AM
akuharev edited edge metadata.

Reduced

rengolin accepted this revision.Jun 26 2014, 5:38 AM
rengolin edited edge metadata.

LGTM,

Thanks!
--renato

This revision is now accepted and ready to land.Jun 26 2014, 5:38 AM

Andrey,
do you need us to commit it?

Thanks for the review!
Could you commit the patch?
I don't have commit access.

-Andrey

rengolin closed this revision.Jun 26 2014, 6:19 AM

Committed in r211776.