- User Since
- Jan 23 2015, 9:38 AM (358 w, 5 d)
Mon, Dec 6
Fri, Dec 3
Thu, Dec 2
You might want to avoid adding new check prefixes in the tests if the sequence is exactly the same. Maybe just change it from CHECK-P9 to CHECK-P9UP. Otherwise LGTM.
Wed, Dec 1
LGTM. The PPC changes are fine AFAICT.
Tue, Nov 30
Seems fine to me but maybe give @MaskRay a couple of days to see if this adequately addresses his comment.
My comments are just minor nits that don't require another review and can be addressed when committing the patch.
Mon, Nov 29
How important is it for the thunk symbol names to match ld.bfd/ld.gold?
Fri, Nov 26
Thu, Nov 25
LGTM. Thanks for the update.
Wed, Nov 24
LGTM aside from a name and return type change.
Please also add a test for this builtin to the front end test clang/test/CodeGen/builtins-ppc-crypto.c
Tue, Nov 23
Just out of curiosity, what does the test case expect? I see mention of Trap() causing a zero vs. non-zero exit code. I don't really follow that since a trap is a void function that doesn't return. But I suppose this is referring to the exit code from the program which I imagine will be something like SIGABRT. Should the invocation of not in the test case just have --crash?
Just out of curiosity, if the csect that contains a function is aligned at 2 bytes, how do we ensure loops within those functions are aligned at 32 bytes?
Although the "one in 10" estimate seems to be a bit inflated, the test case certainly seems to be flaky so I am not opposed to disabling it. Thank you.
Mon, Nov 22
This LGTM but lets give it a few days for others to chime in with their opinion.
Thu, Nov 18
Wed, Nov 17
Mon, Nov 15
LGTM. Thank you.
Thanks for adding these. I look forward to seeing improvements to some of these as there are a number of them that currently produce fairly terrible code.
LGTM other than minor nits.
Please rebase. Does not apply cleanly to ToT.
Please provide a description for this patch which includes justification for why we want to allow conversion between the two types.
I am of the impression that allowing the two types to coexist in completely disjoint code should be fine, but I really don't see a compelling reason to allow conversions between the two types.
Fri, Nov 12
LGTM. There are some very minor nits that can be addressed on the commit.
Wed, Nov 10
Nov 8 2021
Nov 6 2021
Ha ha, yup! Confusion about register classes is kind of a fact of life with PPC's complex overlaying of registers in a single register file. It certainly takes some time to get your mind around FP/VR/VSR/ACC registers.
Nov 5 2021
I am not in favour of this patch. The reasons I added XXPERMDIs a long time ago are:
- To allow a single input operand for single register splat/swap. This is useful when the input is a load (since due to chains, having a load as an input will end up with both loads emitted - i.e. no CSE).
- Since this is primarily useful for loads that load a partial vector (LFIWZX, etc.) the input register class is vsfrc (i.e. all scalar floating point registers).
LGTM other than a number of stylistic changes. Feel free to address those on the commit. You also might want to give @amyk a bit of time to ensure her comments were adequately addressed.
I believe you are planning an update for this patch. Requesting changes to take it off the queue until you have uploaded the updated version.
LGTM. Thanks for fixing this.
Nov 4 2021
Nov 3 2021
Oct 26 2021
Seems like an obvious fix. LGTM.
FWIW, the PPC changes look fine.
Oct 19 2021
Oct 14 2021
Right, preventing optimization around calls due to rounding mode without -frounding-math would be unnecessarily restrictive for most compilations.
Oct 13 2021
Oct 8 2021
LGTM. This is already approved, but just wanted to indicate that I think the comments were addressed adequately.
Oct 6 2021
Please change the condition as Jinsong suggested and change the leading comment to:
// On AIX, only emit the extended mnemonics for dcbt and dcbtst if // the "modern assembler" is available.
Oct 5 2021
Significantly simpler. LGTM, thank you.
Oct 1 2021
Sep 30 2021
Please try out the simpler alternative of letting the legalizer take care of the store size. If that alternative is not possible, please add comments to the patch explaining why it is not.
LGTM as long as the sign-extending test is added.
LGTM. Please run clang-format on the patch (only the modified lines).
LGTM as long as you add the back end test.
LGTM with the test nit addressed.
@saghir Since the bots have been failing for a few days now and there is no response, please try to figure out what commit this is a fix for so we can revert all related commits until we can get to the bottom of this problem.
I may be wrong, but I really think this is incorrect. Please do some functional (execution) testing on this. Also, please re-title this from "truncate results" to something like "truncate exponent parameter" or similar since you are not truncating the result but the parameter.
Sep 29 2021
Sep 28 2021
Added @jansvoboda11 to the review as it appears he was the one that added the original option.
LGTM. Please note in the commit message that this is simply a wrapper for a floating point divide. XL provided this builtin because it doesn't produce software estimates by default at -Ofast.
I really don't understand what happened now. It seems that you have simply reverted to an older version of this patch. The test case appears to not have been pre-committed any longer, the Power9 patterns and test cases are gone. What has happened here?
Thank you for fixing this.