- User Since
- Jan 23 2015, 9:38 AM (453 w, 2 d)
Thu, Sep 21
LGTM. Thanks for reviving this patch.
Thu, Sep 14
Wed, Sep 6
LGTM. The patch is easy to follow now. Thank you for your patience.
Aug 29 2023
The reason 64 seems to be a good value on PPC chips is hopefully a transient situation that seems to have something to do with how bad the chips can be with branch prediction for indirect branches (CTR branches). I think at least some indication should be added in code comments suggesting this be re-evaluated on future HW that will hopefully do better with this kind of branch.
The other knob we have access to wrt. jump tables (density) could probably use some tuning as well.
Aug 23 2023
LGTM. This is a good idea and we should go ahead with this for anyone that uses vec_promote, but it might be a good idea to improve codegen for the insert which might be more common.
I'm not sure I follow. Are you suggesting that we remove all kill flags in this pass? We're really only fixing them up to appease the verifier rather than because anything in the PPC back end relies on the flags.
Aug 22 2023
Aug 20 2023
Thanks for implementing this.
Aug 18 2023
This patch fixes a regression caused by a patch that is in v17. We will need to decide if we should backport this fix or if we should pull the original patch (which also has additional patches dependent on it that are in v17 as well). I think this patch is obvious enough to be backported, but I'll defer the final decision on that to @MaskRay once this is approved.
@amyk Perhaps it would help to perform a thorough test with the approved version of this patch applied to v17 to ensure it is safe to backport.
Aug 13 2023
It seems to me that this section of this function has built up a significant amount of technical debt and we should really try to get it refactored so it is clearer what is happening and why. Please note that this is a statement about the existing implementation, not just about the updates in this patch.
Aug 8 2023
Sorry I missed this. LGTM.
Aug 6 2023
I think the code could be clarified a bit with some additional comments, but otherwise LGTM.
It is not clear how someone in the future should react if the test case changes behaviour. You should think about maintainability of the test cases. It would appear that a part of the checks were produced using a script and a part was added manually. As such, it is not clear what the essential parts of the tests are so anyone maintaining this in the future must really dive deep into the TLS implementation to understand if changes are safe or not. I'm not sure what the best approach would be. Perhaps add a comment describing what is being tested. Or maybe only test the key parts using manual checks.
Seems fine to me - certainly from a PPC perspective.
I don't think the last set of comments were addressed yet. Requesting changes to remove it from my queue.
Aug 3 2023
The patch certainly seems reasonable to me. The test case seems like it could be trimmed down a bit (I don't think all the DWARF directives are needed).
Aug 2 2023
Jul 31 2023
Jul 27 2023
Thank you for the patch. This is actually interesting and is a bit of a change to how we thought the code was do be structured. For us the affinity between CodeGen and MCTargetDesc was an indication that the libraries, although separate, are dependent on each other. Of course, this is not necessary so I am in favour of this refactoring.
Ah, ok. This makes sense. When we don't know anything about the ABI, it makes sense to rely on -mabi.
Jul 25 2023
Jul 24 2023
What version of the PowerISA is this from? What is the motivation for implementing this?
Sorry, I forgot to submit these comments when I reviewed this.
So we don't have code generation for this, but we are enabling it in the front end? What happens if we try to produce code for the IR this produces? It would make more sense to me to implement what is needed in the back end prior to allowing the front end to produce the code.
Jul 23 2023
Please provide a description justifying this change. There is no context here for the viewer to determine whether this change makes sense.
Jul 20 2023
Jul 13 2023
@arsenm Has my description adequately addressed your question? Do you think we should move forward with [some version of] this patch or do you have any fundamental objections?
Jul 12 2023
Lets get this committed to unblock the bots and if it is not the correct/desired fix, the author can subsequently provide the more appropriate fix.
Thanks for adding this. Nice to common-up the behaviour. LGTM.
LGTM. Whatever the choice is wrt. 32-bit mode, please just add a comment to the function explaining it.
Jul 11 2023
No concerns from the perspective of PowerPC here. Of course, my focus is primarily on the server side of things but I am not aware of any other group that could be adversely affected.
What is the plan for handling nearest, away rounding mode for which the PPC FPSCR does not have a setting?
Jul 7 2023
Jul 5 2023
- Run clang-format
- Resolve issue with shift-extend peephole
It appears that this causes failures on buildbots: https://lab.llvm.org/buildbot/#/builders/230/builds/15327
Please fix or pull this.
Jul 4 2023
Yeah, this is because we don't have a way of materializing the <1, 1> vector so we end up with a constant pool load. We can provide custom legalization:
setOperationAction(ISD::SHL, MVT::v2i64, Custom); for Power8 and up where we would just leave the node alone if it's a shift by 1.
At this point maybe just add the test case and we can deal with the issue at a later date.
I agree this can be done in a follow-up patch.
Jun 20 2023
Jun 19 2023
LGTM aside from some minor nits.
Jun 16 2023
Jun 14 2023
May 24 2023
May 23 2023
LGTM. Thanks for fixing these.
May 19 2023
May 18 2023
Please note that this assert can trip even when disassembling with --disassemble and not just -disassemble-all. Namely, the offending word can appear in the text section (for example, the AIX traceback table, etc.).
This is very similar to https://reviews.llvm.org/rGc86f8d4276ae
Apr 28 2023
Rebase on top of some new additions to the pass and address numerous review comments.
Apr 24 2023
LGTM. Thanks for the fix.
Apr 17 2023
Thank you so much for implementing this for PowerPC. Implementing JITLink has been on our radar for a while and this gives us a very welcome head start.
This all LGTM and I think the naming is consistent with other targets, so ppc64 makes sense. I have some minor comments that are more likely due to my lack of familiarity with jit-link than anything that should require action on the patch.
Apr 3 2023
LGTM. Maybe give others a bit of time to chime in.
For the most part, the code produced for vector-promotion.ll is worse as we're forcing the operations into the vector space rather than doing it in scalar registers. Perhaps we should return false from this query if the input vector is the result of a load and the only user is the extract instruction. This would of course require that we change the interface of the query to take the extract instruction rather than the type.
The idea is that if we are loading the vector, extracting an element, performing an operation and then storing, we won't really load the vector but will load only the single element. Promoting the operation will force us to load the vector which is counter productive.
Mar 31 2023
This looks fine to me. I'd like some of the devs that have added builtins with Sema checking in the past to have a look here as well. @amyk @maryammo @kamaub @stefanp Can you have a look at this as well please?
My comments are minor nits that don't require another review, so LGTM.
Mar 24 2023
Mar 21 2023
This LGTM. Thanks for cleaning it up. Give it at least a week for any others that may have an opinion on this to chime in.