LGTM

In D127744#3581771, @reames wrote:

Non blocking comment..

freeze(undef) being equally null or base as undef seems questionable to me. I think in practice it might work out okay, but there's no guarantee that the value chosen for undef isn't 0 when frozen, and 1 when not. You might want to step back and take a deeper look at the problem you're solving here.

@efriedma I've added float->bloat16 conversion tests but I'm not sure these are exact same you had in mind.

Added float->bfloat conversion tests

Submitted an alternative solution (adjusting the shift/exponent)

In D127140#3561467, @efriedma wrote:

I'm not sure it's safe to assume that the lost fraction is lfLessThanHalf, in general. At least, it isn't obvious to me, particularly for cases involving bfloat.

We already have a bunch of code to adjust the shift amount for truncations: the "If this is a truncation of a denormal number [...]" block. Can we adjust that code to reduce the shift amount in this case?

Addressed the comments

hashlib should have deterministic hash functions.

@peter.smith @nickdesaulniers What needs to be done to move forward with this? Do I need to add more procinfo<>feature mappings? Implement a new way to get features entirely?

In D119435#3316830, @tschuett wrote:

This file describes the ABI for exporting the AArch64 CPU ID/feature registers to userspace. The availability of this ABI is advertised via the HWCAP_CPUID in HWCAPs.

and

https://github.com/rust-lang/rust/blob/master/src/tools/rustfmt/tests/source/cfg_if/detect/os/aarch64.rs

So it should be possible from user space.

https://reviews.llvm.org/D92619

In D119435#3316044, @tschuett wrote:

https://github.com/torvalds/linux/blob/master/Documentation/arm64/cpu-feature-registers.rst
Is similar to how Intel is doing feature detection.

In D119435#3313897, @peter.smith wrote:

You'll have to forgive me, I'm coming at this without a lot of context other than what is supported on what Arm architecture, and I didn't have time to reverse engineer it from the code. Thanks for the link, that makes it look like this is related to something like parsing proc/cpuinfo and nowhere else.

If that is the case, would it be possible to update the description with a bit more context, including the link you posted to the available values. Many of the reviewers here won't have the linux context so it can help reviewers if they know what they need to check for.

If this is indeed linux related, may I suggest adding nickdesaulniers as a reviewer. He has been involved in clang built linux project and may know more people that can help from the linux kernel side. If we are going to add more features it would be good to think about what we need to support in total and how to get there across one or more patches.

For testing, given that this is something from the environment I think it would require a unit-test if done on the LLVM side. Making one of those would be good, but quite a bit of work, would be good to put a comment in the description for the benefit of other reviewers.

In D119435#3313720, @peter.smith wrote:

I think we need to be careful here. The extensions in that StringSwitch are for Armv8-a and hence applicable to all AArch64 CPUs. Things like sve are only available on later processors. We'll need to check to see what the intent was here before adding things. We'll also need to work out how to add tests for these, a good idea would be to see how the existing ones are tested.

Got a bit too much on at the moment to look into this in detail. May have some time next week.

Compared to X86, there is a LOT missing (and this hasn't really been updated since ~2014), but at least add a few basic ones.

In D117573#3258469, @samparker wrote:

You mention unimplemented hooks in D60394 and on the github issue, so why did you go for this approach instead? Solving this in a generic manner seems like the 'good' thing to do, no?

I wonder if instead of matching bitcasts, implementing TODOs (hooks) form here https://reviews.llvm.org/D15471 was considered.

LGTM

Changed test to MIR and added STi64 case

Addressed comments

In D107582#3071113, @MatzeB wrote:

The overall issue here is that this pass doesn't seem to correctly handle instructions with implicit defs and tied reg operands. One case of it was fixed in https://reviews.llvm.org/D4351 , and this tries to fix similar issue (see the end of the summary and the test case)

Correct me if I'm wrong, but my impression of D4351 / line 225/226 is that the pass intends to simply not deal with tied-operands at all (regardless of implicit defs/uses being present or not); that seems reasonable enough for me. Though line 226 somehow stopped that logic from kicking in when there are undef operands involved right? So it seems to me the bug happens when you have a tied + undef operand at the same time? I don't see yet how (or if at all) implicit defs/uses play into the picture.

In D107582#3070678, @MatzeB wrote:

@MatzeB Do you have any thoughts on this?

Unfortunately this pass has a very unconventional way to analyze things and manage liveness. It's hard to understand what is going on, so it's hard to judge changes to this pass...

ping

ping

Add more comments to the test

Addressed Quentin's comments

Ping

ping

ping?

Addressed Anna's suggestions

Add missing REQUIRES: asserts

Ping.
I'm tempted to just disable all dep breaking for implicit-def registers, but I fear that it might be too pessimistic for x86_64.

Rebased

ping?

Test failures appear unrelated.

I think I've missed an email discussing this change/deprecation: does it only affect .setAction calls? I.e. if the target only uses builders and doesn't call .setAction could it be affected by this change?

I'm pretty sure this also causes a build failure on older, but supported versions of GCC due to this bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56480):

../tools/llvm-tapi-diff/DiffEngine.cpp:35:71: error: specialization of template<class T, llvm::DiffAttrKind U> void llvm::DiffScalarVal<T, U>::print(llvm::raw_ostream&, std::__cxx11::string) in different namespace [-fpermissive]
                                                     std::string Indent) {
                                                                       ^

LGTM

I remember some noise due to this option (albeit on GCC), because it stops tracking indentation when the number of lines is too big, as it is for files including TableGen'ed inc files. Not sure if there is a way to improve it.

This compilation error (reproducible on GCC 6.2) is still not fixed unfortunately:

../unittests/IR/PassBuilderCallbacksTest.cpp: In member function ‘virtual void {anonymous}::LoopCallbacksTest_InstrumentedInvalidatingPasses_Test::TestBody()’: