[PATCH] D10537: [mips] Add support for branch-likely pseudo-instructions

[Daniel Sanders via llvm-commits]

dsanders added a comment.

In http://reviews.llvm.org/D10537#191677, @dsanders wrote:

> > > Please, take a look at AsmToken::Identifier case, since I am not sure this is the right way to parse a label.
>
> >  You shouldn't need any of the code you added for this. We can already parse labels so specifying 'brtarget' should be sufficient. I'll take a quick look.
>
>
> Hmm... I see the problem. We use a custom parser function (parseJumpTarget()) for jump targets for various reasons. However, it never calls parseJumpTarget() for these branch likely instructions on 32r6/64r6 because the the AvailableFeatures leave no possibility of a valid match. Then, because it didn't call parseJumpTarget() it isn't able to parse the remainder of the instruction and (correctly) rejects the instruction but for the wrong reason.
>
> Fixing this properly is going to be a bit big for this patch. We either need to eliminate custom parser functions as far as possible in favour of a generic parser along the lines of your change, or change the handling of custom parsers such that the parsing happens regardless of the prospects for a successful match. For this patch, I'd suggest omitting the fix for the wrong-error issue.


Hi,

I believe you already have a conditional LGTM for this patch (for diff 28228) and that the quoted text above contains the only remaining issue. It was asking that you remove the AsmToken::Identifier code (because it introduces another way to parse labels which is already handled elsewhere) and fix the wrong-error issue it was intended to fix in a later patch that deals with other cases too.

> PredicateControl moved from CondBranchPseudo to the MipsAsmPseudoInst


I'm not sure why this change was made but it seems to have been reverted in the next diff. I'm not sure if that was intended so I thought I should mention it.

> New diff contain changes that are needed to apply patch to latest version of source code.


This update removed all the context lines so I can't tell what it did.


http://reviews.llvm.org/D10537