User Details
- User Since
- Sep 15 2021, 7:42 AM (71 w, 4 d)
Thu, Jan 26
@nikic, thank you, I'm having a look at these now. Just a few considerations.
- I'm not entirely sure about the unit test failures: should the returned range simply not include zero, if the incoming interval may contain zero and ZeroIsPoison is true, as you noted? I reckoned that being more conservative (unknown/overdefined) could be better here. Same applies for isEmptySet, we may want to return overdefined, I guess (I'm looking at Value of: CR.isEmptySet(), Actual: false, Expected: true)?.
- If we don't want to have ctlz API on APInt, could it be fine to overload TestUnaryOpExhaustive so that it accepts a callable reference to std::optional<unsigned>(const APInt &) too?
- range.ll now fails as the metadata's range is no longer being used now. I'm gradually realizing that this needs to be handled properly in CVP as well. Could it be fine to revert the ret i1 true to this change, considering that the range is now [0,16] (verification here) for this time, and postpone the handling in CVP for a separate dedicated patch?
Wed, Jan 25
Moved the implementation in ConstantRange.cpp, and tested exhaustively. For the sake of completeness and rigor, I left the test in LVI, further improved: it shouldn’t harm, and there are similar unit tests in ConstantRangeTest.cpp which are also in test/Analysis.
Fri, Jan 20
Thu, Jan 12
Wed, Jan 11
Oct 25 2022
@uweigand, thanks for reviewing them! Might I ask you to commit it on my behalf, please (as I do not have commit access, account associated to GitHub)? Thank you.
Oct 24 2022
Oct 22 2022
Hello @uweigand, I apologize for being late on this. From the build, it looks like some systemz tests are failing because the register name is actually not emitted. Is it possible that I'm not handling properly it via SystemZInstPrinter::getRegisterName? Do you have any clue by any chance? The duplicated code in SystemZAsmPrinter.cpp looks correct to me.
Sep 19 2022
@MaskRay, as there do not seem to be further concerns, could you kindly commit this on my behalf please (I do not have commit access)?
Sep 13 2022
@MaskRay, test added for this patch too, failure of unit tests seems completely unrelated to the change.
@MaskRay, fixed by extending properly the patch. Coverage test preserved, and added a further one for completeness.
Sep 11 2022
Sep 10 2022
@MaskRay, rebased to trunk & added more coverage test!
Sep 2 2022
@MaskRay, yeah, sorry, I should have probably specified it :/ Essentially, the other commit 4e9907977465 was relying on this one, so this one should have been pushed before, as the other contained the test (which leverages on this change too). Hence, there should be no need to actually introduce tests for this one, as already introduced in the other commit.
Aug 28 2022
Aug 27 2022
Fixed a small regression.
Aug 26 2022
@MaskRay, test added!
@uweigand, mostly third-party clients such as disassemblers and pretty printers, as per official documentation. Already supported by ARM as well as x64.
@MaskRay, test added!
@MaskRay, test added!
Test added.
Aug 25 2022
/cc @MaskRay
Aug 20 2022
@david-arm, thanks for the nit, had missed this. I just updated the diff w/ that change included. Mind committing it on my behalf (GitHub account associated to this profile, as I do not have commit access)? Thank you.
Jul 15 2022
Jul 13 2022
Jul 12 2022
Apr 7 2022
Awesome, thank you for reviewing this! PS: I do not have commit access, feel free to close it, if you say.
Apr 6 2022
Fixed the change, and added a new test. Hope the coverage is at a fairly good level now!
Apr 5 2022
@probinson, sorry for late reply, it turns out that the sub-project, which this change was needed for, has been recently dismissed, so no longer needed. Also, we did not have any change of ours to provide, alongside the patch. That said, considering that the inverse function parseArch already supports s390x and the proposed change is fairly minimal (besides that getArchTypeForLLVMName is already tested via lookupTarget), I'm still wondering if part of the patch (at least the case adding s390x arch) could be beneficial. If you think it's not, I'll close the review.
Apr 4 2022
Looks definitely better! How about this slightly changed version protecting the interface?
/// Helper which adds two underlying types of enumeration type. template <typename EnumTy1, typename EnumTy2, typename UT1 = std::enable_if_t<std::is_enum_v<EnumTy1>, std::underlying_type_t<EnumTy1>>, typename UT2 = std::enable_if_t<std::is_enum_v<EnumTy2>, std::underlying_type_t<EnumTy2>>> constexpr auto addEnumValues(EnumTy1 LHS, EnumTy2 RHS) { return static_cast<UT1>(LHS) + static_cast<UT2>(RHS); }
I feel like this is something we may wish to readopt in the future for other similar cases as well (e.g., -Wdeprecated-anon-enum-enum-conversion in Comment.h).
Mar 31 2022
Mar 22 2022
I have updated the patch by removing the case for x86_64. Although it looks like that's a known problem in LLVM (converting the arch-name to Triple::x86_64 and then back to string does not return the original name), this may warrant a separate fix; so that, should something break up, we may easily revert it, and not blame this change.
Mar 18 2022
@dblaikie: That's true, admittedly, had not noticed the protected dtor (which works as well). Fixed it up, thanks for contextualizing.
Sep 19 2021
I don't have commit access, may anybody close this, please?