- User Since
- Feb 5 2014, 1:36 AM (214 w, 5 d)
Tue, Mar 6
Mon, Mar 5
Thu, Mar 1
Tue, Feb 27
Wed, Feb 21
LGTM, but please wait a day to let other time-zones comment.
Feb 16 2018
Feb 14 2018
The test case doesn't have to be exactly the -O0 output from clang, you should reduce it to clear IR which triggers the bug. To start with, are all of those allocas and memcpy calls required?
Feb 13 2018
Feb 12 2018
To have a better handling than this, we'd have to error out earlier saying M-class doesn't have NEON, which probably needs more knowledge than that piece of code has...
Geoff Berry suggested to do this in DAG Combine
Ping (also for the related D42478).
Feb 9 2018
Add a triple to the test.
Feb 8 2018
Was only wondering about the Cortex-A8 -> A15 change in the test run line.
Feb 7 2018
LGTM, but please wait a day to let other time zones comment.
Feb 6 2018
Would it be possible to enable this for all targets, not just ARM? Maybe this could be put into the constructor of LLVMTargetMachine?
Feb 5 2018
LGTM with a few nits (see inline comments), but please give 24 hours for other time zones to comment before committing.
Feb 2 2018
Feb 1 2018
Jan 31 2018
Thanks for making these changes, LGTM.
Jan 30 2018
I'm still not convinced about the correctness of this transformation: you are turning code which contains truncates and extends into code that doesn't, without checking whether the top 16 bits could be relevant. This happens to be OK if the value is coming from/going to an fp16 arithmetic instruction, which ignores/clears the top 16 bits, but I don't think it's correct in all cases.
Jan 29 2018
Is your concern that I am changing the semantics of these nodes because I am omitting this convert?
It might be intended to only apply to function arguments and returns, but those patterns for f16_to_fp and fp_to_f16 could match anywhere.
Have you tried adding tablegen patterns for bitconvert nodes between i16 and f16? That's how it currently works for f32<->i32, see the VMOVRS and VMOVSR instructions in ARMInstrVFP.td.
Jan 25 2018
Ok, I agree with the idea of committing this as a starting point and developing it gradually. Just a few nits left.
Jan 24 2018
Jan 23 2018
Jan 19 2018
Well, this patch doesn't change the actual conversion to MCInst, it just encodes more information into the conversion-function by using an indirection through the 'TiedAsmOperandTable'. It will still create the MCInst in the same way as it did before.
I was looking at your original version of this the other day to work out if it could be used to do 3-operand aliases of 2-operand Thumb1 instructions, and this (adding support for aliases) looks like it's what we would need for that.
Jan 17 2018
I've spotted a missed optimisation in the tests, but that should be done as a separate patch.
Jan 16 2018
This looks like a lot of additional complexity to deal with the case where we only have the conversion instructions, but you have made f16 a legal type.
Jan 5 2018
I approved this patch back in November.
Jan 4 2018
This is described in the "Instruction endianness" section in the architecture reference manual, which is section A3.3.1 in the latest version of the v7-A/R manual. Here's the relevant bit:
Dec 13 2017
Dec 4 2017
This is very similar to what I'm doing with the multiple-near-miss diagnostics, trying to only emit messages which point out exactly what is wrong with an instruction, rather than simply the first thing the assembler found wrong. Hopefully this will be made obsolete once I get the near-miss mechanism completed and ported to non-ARM targets, but this looks like a reasonable point fix in the meantime. LGTM.
I've reverted this because it is causing a buildbot failure - http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/6552/steps/test-check-all/logs/stdio.
Dec 1 2017
Could you also add a test for the (i32, i64) case? We can currently emit the tail call for that (because r1 is not used).