@nikic, thank you, I'm having a look at these now. Just a few considerations.

1. 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)?.
2. 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?
3. 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?

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.

Following thorough investigation, it turns out that the introduced interval range refinement has already been covered in db9d00c by @nikic. Closing this, sorry for the incident.

@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.

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.

@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)?

@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.

@uweigand, we leverage LLVM disassembler (and, specifically, MCInstPrinter::printInst) as part of our static binary translator tool, here, though I assume I could count many other uses that I'm not aware of (we've updated other targets too in order to support markup).

@MaskRay, rebased to trunk & added more coverage test!

@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.

Fixed a small regression.

@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.

/cc @MaskRay

@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.

Awesome, thank you for reviewing this! PS: I do not have commit access, feel free to close it, if you say.

Fixed the change, and added a new test. Hope the coverage is at a fairly good level now!

@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.

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).

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.

@dblaikie: That's true, admittedly, had not noticed the protected dtor (which works as well). Fixed it up, thanks for contextualizing.

In D122003#3392424, @probinson wrote:

Test? I'm a little surprised that changing the spelling of an arch didn't cause any failures.

I don't have commit access, may anybody close this, please?

In D109888#3004544, @nikic wrote:

I'm okay with this for the case of function attributes in particular, because those don't really depend on ABI considerations. I will say though that you're brushing really close to UB here, and I may push back against extending other places in this direction. Like, say you have a void(i128) and call it as void(i64, i64) knowing that under the particular ABI used those two i64s will assemble into an i128 -- is that a legal call? In what manner would a function param attribute correlate with call arguments there?