Added more reviewers with history of the modulo expansion code, since I believe there is no feasible test for upstream targets, see commit message.

Reverted the patch in repo and re-pushing again. The only difference of the new push is s/enum_seq_inclusive/enum_seq/ in TargetLoweringBase.cpp.

Rebase only.

In D126386#3537423, @hgreving wrote:

There are a few lit failures, I will look at those first.

Updates patch. Let's do it straight forward and support cases where no branch is needed to the fallthrough.

There are a few lit failures, I will look at those first.

In D125574#3533949, @fhahn wrote:

In D125574#3532385, @Meinersbur wrote:

LGTM. Please wait for @fhahn LGTM as well.

It looks like the patch was landed but the commit message seems a bit out-of-sync with the code.it

In D125696#3532788, @aeubanks wrote:

precommitting a test for an approved patch generally shouldn't require review

Friendly ping on this review.

Friendly ping on this review.

In D125574#3516111, @hgreving wrote:

In D125574#3515105, @fhahn wrote:

Thanks for the patch! Makes sense, I added some suggestions with respect to the test.

Should be done.

In D125574#3515105, @fhahn wrote:

Thanks for the patch! Makes sense, I added some suggestions with respect to the test.

Rebased to https://reviews.llvm.org/D125696

Whoa another corner case.

Added a few clang-format on/off. I suggest to accept some of the enum changes in IntrinsicEmitter.cpp, Function.cpp, it feels unnecessary not to accept clang-format here. I wonder if these deltas are due to different clang-format versions in the past.

In D125247#3503920, @craig.topper wrote:

Please upload patches with full context using git diff -U999999

Full context update (excl clang-format off notations yet)

In D125247#3503918, @craig.topper wrote:

In D125247#3503854, @hgreving wrote:

clang-format changes. I am aware that some of the changes look like they do not follow existing code. I'm not sure why clang-format does this.

We've been ignoring clang-format on these files. Maybe we should add // clang-format off markers

clang-format changes. I am aware that some of the changes look like they do not follow existing code. I'm not sure why clang-format does this.

Abandoning, may try the integer route or check the sel DAG failures more closely.

In D125018#3498099, @craig.topper wrote:

What SelectionDAG support is needed for i2/i4?

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079386, @arsenm wrote:

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079188, @hgreving wrote:

The MMI should be reflecting the underlying module. Why isn’t your IR expressing you don’t want the debug info emitted by setting the emission kind to NoDebug?

Short answer: I didn't know about this, interesting, thanks! Longer answer: the IR is from MLIR, not sure if they can set this accordingly, but we can make this work. So the emission kind is the way to preset?

Yes. It leaves the debug metadata around for the various remarks and things that want it, but doesn't emit it in the final binary (part of what this happened to fix was the handling of this for CodeView). Alternatively you could just strip the debug metadata if you really don't want it.

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079187, @arsenm wrote:

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079166, @hgreving wrote:

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079165, @arsenm wrote:

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079156, @hgreving wrote:

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079154, @MaskRay wrote:

@hgreving reported a concern that the removal of setDebugInfoAvailability may not have an alternative. I am just forwarding the message here :)

We're using this switch downstream to suppress the asm printer complaining about debug symbols not being absolute (we have a simplified linker backend). Since the backend has its own MMI module I don't see how you can preset debug availability. Is this change really necessary, I wonder who else was using this switch?

I'm trying to get MIR printing into a more reliable state, such that you will get the exact same result if you stop and restart the compile at any point. MachineModuleInfo is not serialized now, so any dynamically changed state here is discarded. I do not believe it's appropriate for a codegen pass to be dynamically setting anything here. Everything here is really a cache, or a module level analysis (which could and probably should be split into separate analyses) and should not be stateful.

What do you mean exactly by "preset debug availability"?

I see. I was referring to "This can be set up front" in the commit message which seemed to suggest one can preset the same flag upon initialization? If so, I didn't see where. Basically our linker is extremely simple, setting all symbols to a variable value which makes them absolute. If MMI.HasDebugInfo(), the asm printer will complain about such debug related symbols being absolute. It looks like we can use -disable-debug-info-print, but I liked the previous API better. We're setting this in the asm printer pass, lacking a better place.

The MMI should be reflecting the underlying module. Why isn’t your IR expressing you don’t want the debug info emitted by setting the emission kind to NoDebug?

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079165, @arsenm wrote:

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079156, @hgreving wrote:

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079154, @MaskRay wrote:

@hgreving reported a concern that the removal of setDebugInfoAvailability may not have an alternative. I am just forwarding the message here :)

We're using this switch downstream to suppress the asm printer complaining about debug symbols not being absolute (we have a simplified linker backend). Since the backend has its own MMI module I don't see how you can preset debug availability. Is this change really necessary, I wonder who else was using this switch?

I'm trying to get MIR printing into a more reliable state, such that you will get the exact same result if you stop and restart the compile at any point. MachineModuleInfo is not serialized now, so any dynamically changed state here is discarded. I do not believe it's appropriate for a codegen pass to be dynamically setting anything here. Everything here is really a cache, or a module level analysis (which could and probably should be split into separate analyses) and should not be stateful.

What do you mean exactly by "preset debug availability"?

In rG9592e88f59cfd399e9285cfec50a23675f43a43a#1079154, @MaskRay wrote:

@hgreving reported a concern that the removal of setDebugInfoAvailability may not have an alternative. I am just forwarding the message here :)

In D122203#3399682, @nikic wrote:

LGTM. Just to be clear, when you say "vector pointer" you're talking about a "pointer to vector" rather than a "vector of pointers", right?

In D122203#3398701, @aeubanks wrote:

Could you make the title more specific, mentioning intrinsic overloading and opaque pointers?

Also could you add a FIXME saying that other nested types don't properly work with intrinsic overloading but hopefully it doesn't happen in practice?

In D110026#3012843, @hgreving wrote:

In D110026#3012827, @nikic wrote:

In D110026#3012747, @hgreving wrote:

Seems like that this patch now contradicts the methods MDBuilder.h:createAnonymousAliasScopeDomain and createAnonymousAliasScope. If you actually give it a string, it won't verify. I wonder if you should amend this changing the methods as well?

These methods are intended to be supported and are used in-tree and do verify there. Could you provide an example that would fail? Please keep in mind that scoped alias metadata always deals in scope lists, while createAnonymousAliasScope() only creates an isolated scope. You cannot pass the result of createAnonymousAliasScope() directly to !noalias, !alias.scope, etc.

Yes. But MDB.createAnonymousAliasScopeDomain("mystring") noew creates e.g. the domain !33 = distinct !{!33, !"mystring"}, which leads to the verifier error "second scope operand must be MDNode". This seems inconsistent. Same happens for createAnonymousAliasScope().

In D110026#3012827, @nikic wrote:

In D110026#3012747, @hgreving wrote:

Seems like that this patch now contradicts the methods MDBuilder.h:createAnonymousAliasScopeDomain and createAnonymousAliasScope. If you actually give it a string, it won't verify. I wonder if you should amend this changing the methods as well?

These methods are intended to be supported and are used in-tree and do verify there. Could you provide an example that would fail? Please keep in mind that scoped alias metadata always deals in scope lists, while createAnonymousAliasScope() only creates an isolated scope. You cannot pass the result of createAnonymousAliasScope() directly to !noalias, !alias.scope, etc.

Seems like that this patch now contradicts the methods MDBuilder.h:createAnonymousAliasScopeDomain and createAnonymousAliasScope. If you actually give it a string, it won't verify. I wonder if you should amend this changing the methods as well?

In D104801#2890876, @tra wrote:

In D104801#2889283, @hgreving wrote:

Besides that this patch created the intrinsics llvm.memset which in our target was unsupported, it also created pointers that may be invalid on the target, e.g. i8 addrspace(X)* where addrspace(X) only supports vector memory. While this is probably all solvable, please consider adding TTI switches for targets to disable this optimization (e.g. for the intrinsics).

...and that brings us back to where this patch has started -- with a TTI knob to control whether it wants this pass enabled. :-/

That said, I'm not quite sure that "this type/AS combination" is invalid is something you can expect all parts of LLVM to respect. While this pass may happen to tickle it, I can see other cases where the type of the pointer may change. I would think that dealing with the quirks of type support of particular target would be something for legalizer to do.

Besides that this patch created the intrinsics llvm.memset which in our target was unsupported, it also created pointers that may be invalid on the target, e.g. i8 addrspace(X)* where addrspace(X) only supports vector memory. While this is probably all solvable, please consider adding TTI switches for targets to disable this optimization (e.g. for the intrinsics).

In D104576#2828828, @jmorse wrote:

Just so that I understand clearly, the fault here was modifying operands while using range-iteration over the same collection? Was the symptom a crash, or missing output? (Thanks for the catch).

Second CHECK line.
clang-format.

Use iterator preincrement instead.

In D98240#2781262, @fhahn wrote:

In D98240#2781048, @hgreving wrote:

For targets not supporting scalar load from vector memory (like ours), this breaks it:

%43 = load <8 x i32>, <8 x i32> addrspace(201)* %1, align 32, !tbaa !28
%44 = extractelement <8 x i32> %43, i32 0

Now:

%43 = getelementptr inbounds <8 x i32>, <8 x i32> addrspace(201)* %1, i32 0, i32 0
%44 = load i32, i32 addrspace(201)* %43, align 32

Are targets expected to provide patterns?

Interesting! I guess the code assumes that a scalar load is always possible & at least as cheap as the vector version. But I think it would make sense to ask the cost-model if that's the case. Not sure if it would be possible to test this with an in-tree target?

For targets not supporting scalar load from vector memory (like ours), this breaks it: