@lebedev.ri : I think I need your blessing as well on this, considering your earlier concerns. Is it still just confusing? (I doubt that we want/can replace all uses of getCharWidth/getIntWidth etc in clang with integers)

Still LGTM

Updated to add CharTy to the CodeGenTypeCache (based on review feedback).

In D92309#2509524, @foad wrote:

In D92309#2509390, @bjope wrote:

LGTM

Thanks. This patch depends on D92308. Without D92308, this patch would add lots of extra lines to the output of opt -debug-pass=Structure, which breaks various tests in test/CodeGen and test/Other.

LGTM

In D94979#2507242, @lebedev.ri wrote:

As far as i recall, all RFC's about LLVM support for non-8-bit char targets didn't end in favor of the proposal
I'd say that until there's general consensus/buy-in (including testing path), cleaning stuff up will only confuse things.
CC @jfb

In D94446#2492844, @anton-afanasyev wrote:

Looks good, but could you please precommit test (and rebase) to see actual output difference?

I've now tried to do some performance comparisons using perf stat -r 100 opt -O3 -o /dev/null --verify-dom-info -verify-assumption-cache -verify-loop-info.

I created this patch as my earlier patch (https://reviews.llvm.org/D94138) resulted in even more duplicates in the preserved sets. So the idea was to compensate a bit for a potential speed regression, when using verifiers, by simply getting rid of all duplicates in the sets.

In D94416#2493099, @foad wrote:

Does this make any difference in practice? E.g. does the output of opt -O1 -debug-pass=Executions change, or can you measure any timing difference?

Rebase after pre-committing the test case.

I landed this on behalf of @ebevhan

I've just landed this on behalf of @ebevhan.

In D54749#2489043, @nikic wrote:

@bjope There are two more revisions based on this one, D86079 implements improved X86 lowerings and is already accepted, and D86078 implements improved AArch64 lowerings. Do you plan to land these as well?

Ping!

There is a bunch of lit tests that fail if adding a DT->verify() check in isKnownNonZero.

In rGb21840751278128ef6942074ae89ea63c9c6ac58#967637, @uabelho wrote:

Hi @nikic,

I've seen a miscompile for my OOT target with this patch. Unfortunately I haven't gone to the bottom with it yet (and I don't think I will until after new years), so I'm not sure what the problem is exactly, but I think that AA comes to the conclusion that a read and a write to the exact same location doesn't alias, and then things go wrong. Are you aware of any problems?

In D54749#2458355, @rjmccall wrote:

I'm not entirely sure what you're still waiting for, really. It's a big patch with a lot of diffuse responsibilities, but you've gotten sign-off from individual people across at least most of it. Do you feel like there's something significant that hasn't been reviewed?

In D86203#2453302, @arsenm wrote:

In D86203#2445098, @bjope wrote:

But if I understand this patch correctly it has tried to derive the type from "some_isnt" when it is omitted, rather than reusing the type from the source pattern. Would it make more sense to infer the type from the source pattern instead of from the target instruction when it is omitted? Is that possible?

The problem isn't the type, it's the register class. It can be ambiguous to go from the target instruction back to the source bank if the target register class supports multiple banks

Gentle ping again.

But if I understand this patch correctly it has tried to derive the type from "some_isnt" when it is omitted, rather than reusing the type from the source pattern. Would it make more sense to infer the type from the source pattern instead of from the target instruction when it is omitted? Is that possible?

@SjoerdMeijer : I've reviewed the non-target-specific changes now. And that part looks good to me now.

Add back test cases (they were accidentally removed when commandeering this patch and rebasing earlier).

Big thanks to @ebevhan for doing lots of work moving forward with the fixed-point number (embedded-c) support.

Address review comments from RKSimon.

Rebase

Our "master integration" also stopped after this commit, and we hit the LIBCXXABI_LIBCXX_INCLUDES= is not a valid directory. error. We've never provided any LIBCXXABI_LIBCXX_INCLUDES so I'm not sure what I should point it to.

As I see it this solution focuses on the problem seen in LoopUnroll. So I agree, it is conservative and simple so if it solves the PR I think it is just fine.

In D88928#2316194, @fhahn wrote:

So: skip the call to RemoveRedundantDbgInstrs. There's surprisingly little fallout from this, and most of it can be addressed by doing RemoveRedundantDbgInstrs later.

As an alternative, would it make sense to have a dedicated pass to eliminate redundant debug instructions, that's run late in the pipeline? not sure if running it once, but for every function will be better or worse in the end. It should be easy to skip for functions without debug info though.

One thing that I've not understood is how GlobalISel is different from the legacy ISel here. Apparently it isn't necessary to annotate things with the legacy ISel today? So is legacy ISel doing the same thing that this patch suggests to do also for GlobalISel, or why do we suddenly need to update all patterns now (I mean, somehow it has worked fine in the past, right)?

In D88454#2309456, @phosek wrote:

I think I figured it out, D88756 should address the issue.

Looking at build.ninja we used to get

I've checked the late "better safe than sorry type checks" and they LGTM.

In D88454#2308086, @phosek wrote:

In D88454#2307806, @uabelho wrote:

Hi!
I don't understand why, but with this commit I start seeing several warnings like this when compiling builtins:

/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:45:7: error: '__sanitizer::FlagHandler<bool>' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]
class FlagHandler : public FlagHandlerBase {
      ^
/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:71:13: note: in instantiation of template class '__sanitizer::FlagHandler<bool>' requested here
inline bool FlagHandler<bool>::Parse(const char *value) {
            ^

That's a bit surprising. What's your build configuration? Do you use runtimes build? I haven't managed to reproduce it locally so far.