- User Since
- Jun 19 2015, 4:53 AM (148 w, 2 d)
Fri, Apr 20
Thu, Apr 19
One major comment inlined, I'd like one of the other reviewers to chime in if it's the better solution.
A quick comment on the error message, inlined. It's about the quality of the diagnostics.
Looks like I missed some instructions. I'll update this patch shortly.
I think we need to modify LegalizeDAG .cpp here directly to eliminate the multiplication, as the legalizer has legalized the ISD::BR_JT into an address synthesis, load and multiplication. The legalizer then recursively legalizes the multiplication into a mult/mflo pair.
Tue, Apr 17
Can you re-upload this with full context?
Fri, Apr 13
Only some minor nits now, inlined.
Thanks for the review.
Thu, Apr 12
Wed, Apr 11
Ok. The translation is covered by the irtranslator tests, so we should catch any unexpected changes.
The LLVM-IR in the MIR test files doesn't match the MIR below, but I haven't checked if that makes a difference.
Tue, Apr 10
Fri, Apr 6
You should also update the description of the patch.
Thu, Apr 5
Wed, Apr 4
I don't think this is quite the correct approach, especially if you're going to be implementing support for 64-bit pointers with those instructions, as we would need to modify the existing lb64 and friends to be available to the AsmParser with a different set of operands.
This looks mostly ok. The big changes required is that these instructions should go in the relevant base architecture .td, as I don't believe there's enough of them to warrant going into two separate files.
Thanks for the review.
Tue, Apr 3
Mon, Apr 2
Thu, Mar 29
Hi Milena, sorry for the delay in getting back to this, I', currently on holiday but I will get back to looking at this sometime next week.
Mon, Mar 26
LGTM, I'm not seeing an implementation of that method or a callee.
Mar 20 2018
Some small nits inlined, I've mostly looked at the non debug related parts. A lot of my comments are the MIPS .td parts can be summarized as: where possible, modify the underlying *_DESC or MXC1 classes where possible rather than wrapping the definition of the instruction with a 'isRegMove = 1'. There are some cases where it can't be done as the instruction description class is shared multiple other instructions which are not moves.
Mar 19 2018
@RKSimon , I'm ok with this going in, I'm in process of updating our scheduler models to account for this but it may take me a few days.
Thanks, I wasn't sure who to add as a reviewer.
Mar 16 2018
Mar 13 2018
Update to remove unrelated changes to trap instructions.
LGTM with inline nits addressed.
This patch looks mostly ok, but I think there are some small issues with it.
Mar 12 2018
Mar 9 2018
Update to account for splitting out one of the changes into D44299.
Minor update, incorrect predicates for (s|l)w(l|r)e, fixed the ASE_EVA class overwriting the InsnPredicates instead of the ASEPredicates.
If you look at the dependency, you'll see the way I've separated out the ISA level from the ASEs.
Based on my comments on D44176, I'm preparing another patch which is the splitting out of the ASEPredicate from InsnPredicates in PredicateControl.
Mar 8 2018
Mar 7 2018
This looks mostly ok. There's only some small changes required, and they are somewhat minor. The recurring change is that for test cases, when there is a run-on line with '\', then the continuation of the command line should be indented by two spaces.
Mar 5 2018
Some comments on the MIPS part of this patch inlined.