- User Since
- Jul 31 2018, 10:55 AM (112 w, 2 d)
Aug 21 2020
Aug 20 2020
Jul 15 2020
Sure I'll land these apologies for the delay..
Jul 14 2020
See nitpick, otherwise LGTM
See nitpicks, otherwise LGTM
Thanks Paolo, tests are all passing and apart from the nitpicks this is a green light from me, as with the rest in this series.
Jul 9 2020
Jun 28 2020
Bumping this patch - Looking at the output of the riscv-expand-pseudos pass I'd say these values are accurate. I'd say comments here, and in the functions which expand the instructions to ensure things don't get changed - seems to me like tests would be a little tricky so perhaps omitting that would be fine just to get this landed.
Apr 18 2020
Bug fix - ensure selectConstant produces copies with fully constrained registers.
Bug fix - don't try to access the size of a $noreg register.
Add target flags to calls in tests.
Bug fix - add target flags to call instruction.
Apr 17 2020
Add custom selection for copies and for constants. Significantly expand tests over more types.
Add more operand mappings and expand tests over more types.
Tweak test order
Apr 16 2020
Add testing for shifts. Include a version of handling single word operations on RV64 involving directly lowering to the final opcode.
Apr 13 2020
Expand patch to include operations for other types as introduced by the legalizer.
Apr 12 2020
Expand patch to correctly cover more methods of types being legalized - IE: single word instructions on RV64, split operations (add with carry etc.) and libcalls up to s128.
Apr 10 2020
Removed unnecessary line
Apr 7 2020
Add tests for AND/OR/XOR.
Apr 1 2020
Mar 30 2020
Address comments relating to supported types.
Mar 29 2020
Rewrote the global RegBanks array to be constant. Instead of modifying RegisterBanks after construction, provide HwMode to the constructor of RegisterBankInfo and move size calculation logic for RegisterBanks to that class.
Mar 26 2020
I'm lacking an understanding of how concurrency is expected to work within LLVM, but since there is only one RegisterBankInfo constructed per subtarget I don't see how there is a problem with updating the global RegBanks array? Likewise the HwMode is constant for the subtarget, so the write will be of the same value.
Mar 23 2020
My initial thoughts are that this may only be an issue if there can be potentially more than one instance of RegisterBankInfo? And indeed, if it is possible for each of these to have been constructed with differing HwModes? I'll try and look into it in more detail and find out.
Mar 19 2020
Add XLenLLT, take into account M extension.
Mar 18 2020
Add test for pointer return type
Fix copy/paste mistakes in check lines
All looks good apart from some test nitpicks. Also add more reviewers
This looks good to me, as long as it covers the spec. Can you add more reviewers though?
Thanks, this is looking in good shape now. As long as everyone agrees on this scheme I think the implementation is good to go (pending the bitmanip extension support).
Mar 12 2020
Mar 11 2020
Mar 5 2020
Rebase, update test structure.
Rebase, update test structure. Correct missing defs by adding CallReturnHandler.
Feb 23 2020
Feb 22 2020
Feb 21 2020
Feb 12 2020
I can't see any issues with the logic of this implementation. It doesn't lend itself well to adapting it in the future if we want to be able to combine the operators, but I'd expect that would require a more disruptive rewrite so I'm not too concerned about that right now. See comments though.
Feb 11 2020
Since the DebugInfo fix has been accepted, I'm looking to get this patch and that fix committed shortly if there are no problems caused by rebasing.
Feb 8 2020
Feb 5 2020
Updated comment and added brief testcase, which now depends on D62686
Feb 4 2020
Thanks all for the feedback, I'll make this patch depend upon the save/restore feature so that I can add a testcase.
Thanks for addressing the comments, I'd like some input from others before saying this looks good.
Well spotted, this seems like a good change to me. I wonder if there are other optimization opportunities to use this NegImm/simm12_plus1 pattern in further patches?
Thanks, this seems like a good change. I haven't checked the source, but I wonder is it ever possible this hook triggers duplication of the returns but the tail calls don't get emitted for whatever reason? Other than that and the comment above I'd be happy with this.