looks reasonable to me

The context of this work was to recognise loop induction statements in Machine Instructions/Loops and to allow calculation of loop iteration counts. And isAdd provides more semantic information about a MI and helps recognising induction variables. Thus, this was the ground work for D27193, which was the next step to recognise loop induction. This patch was abandoned because the Quentin's comment was that the analysis was too ad-hoc and missing some formalism. The reason this is used only in the Hexagon backend, is because I refactored it and took it from the Hexagon backend. The Hexagon backend has a lot of MachineLoop/MI analysis and other goodness (sw pipeliner) that is mostly target independent and could be made available, but a few target specific hooks are required to do this.
Now, I really would like to pick up this work again because I still think it will be valuable, but need to rethink strategy after abandoning D27193. Probably that will be a some SVEC-like approach, but hopefully that can be a light-weight implementation to recognise simple loops (which is what we're doing anyway). This will probably be quite some work, so in the mean time, I really don't mind removing or moving isAdd back to the Hexagon backend. I could also dig up the exact requirements and document that if it's okay to keep it.

Thanks for checking!

I was just replying, but yes, there are only 4 bits available to encode Vm, the other M bit is the index.

Hi, the full architecture specification is publicly available here:

Looks reasonable to me.

many thanks for reviewing and your help.

Fixed the typos, and added tests.

Many thanks for reviewing and your help!

I am going to commit this within a few days. That looks reasonable to me given that the comments in the last reviews were very minor (which I have of course addressed already). Also, in case of issues, I am guessing fixes and/or addition can be easily done post-commit, and if not a revert is cheap.

Hi Sam, many thanks for refactoring this. Looks really good now! Just 2 nits inlined, but no need for another review.
Cheers, Sjoerd.

Comments addressed. Thanks for reviewing.

No changes were needed to make the conversions work, the existing logic is taking care of that, but I agree it doesn't hurt to add a few test cases. So I've added tests to both files, and cleaned up that comment.

Thanks for reviewing! I've addressed your comments: have set operation action to 'promote', added checks to test fcmp, and also added checks when fp16 is disabled.

Fixed typo in test case type declaration.

Forgot to mention that I will address the v8f16 types in a separate patch.

Thanks for fixing this.

lgtm

Added test case.

Looks good to me, was wondering if you're also planning to add a Clang patch to test +rdm?

Just to clarify my last comment, I will repeat the exercise I did in D36396 allowing f16 scalars, and will now start working on allowing f16 vector types. I will then also add tests for these.

I have removed the special case, and added the vector types.
For the copysign intrinsics working on these vectors, I have not yet added new test cases, because it looks like I first have to first do some work to allow f16 vectors.

thanks for reviewing!

Added missing spaces.

Hi Alex, thanks for the feedback. The difficulty here is finding the right balance of adding (good) debug messages, and not cluttering the code too much. I think what you are suggesting is what I had in my first revision: that had debug messages in the different AArch64 lowering functions. Then Sam's suggestion was to move some of these messages to SelectionDAG (which we did in D36984), which has 2 advantages: all targets benefit, and it does not clutter up the code. Also, I think this is quite difficult to get completely right the first time. So I think we will have to see what works and what is good and I will be adding more stuff as I go along. There's a lot of room for improvement here, but I already find these extra debug messages quite useful...

Removed the debug messages that were moved to SelectionDAG in D36984

Hi Sam, thanks for reviewing. I have addressed your comments. Cheers.

Just added a few more debug messages.

I will split this patch up in a target dependent, and target independent patch. I will continue here with the AArch64 specific messages, and I have created D36984 for some of the debug messages that I moved to SelectionDAG getNode functions.

The disadvantage of moving it to SelectionDAG is that we lose the context where the new nodes are created. In other words, the debug message will (potentially) become less specific. But since they currently do not have an awful lot of context, I think your suggestion makes sense. I will experiment to see if it works.

Looks reasonable to me.

Looks good to me too.

Ah, my message crossed with Renato's; I only noticed it after submitting mine (looks like we agree though:-))

"They also implement the RCpc AArch64 extension from ARMv8.3-A."

I forgot to address some of Oliver's earlier comments:

• I have added fixes for the conversion functions, and modified the tests,
• I will address the codegen for copysign in a follow up patch, because it it requires a bit of custom lowering.

Ping

Hi Sam, Oliver, thanks for checking and reviewing. Your gut feelings were right: some more testing showed that target hook isFPImmLegal was not allowing f16. So I've modified that function, and added more tests.

thanks!

Hi Sam,
Thanks for reviewing. I agree it looks odd, but the argument of not doing this, is that this is not user-facing. I.e., they are internal options and it's Clang's responsibilities to parse options and set the target features. Other optional instructions/extensions are also not predicated on e.g. v8.1a or v8.2a.

Added the disassembler tests.

Oops, I now see that I forgot to add the disassmbler tests.

Looks okay to me.

Thanks for the review!

Looks reasonable to me.

Probably the subject/description of this patch is a bit confusing/misleading. I should have mentioned in the description that all the groundwork of adding the ARMv8.2-A FP16 (scalar) instructions was done D15014, which added all the instructions, and also e.g. an fpimm16 immediate type, and the required regression tests for these instructions. This patch is just a tweak to avoid promotions for FP16 instructions that support f16 operands.