In D86233#2256909, @hfinkel wrote:

In D86233#2256908, @nikic wrote:

Or might_not_progress while we're at it :) No requirement to squash the whitespace.

True. Although we squash the whitespace for all other attributes. I don't really like it, and would prefer the underscores, but it would be different.

In D86233#2256908, @nikic wrote:

Or might_not_progress while we're at it :) No requirement to squash the whitespace.

In D86233#2256872, @lebedev.ri wrote:

FWIW maynotprogress reads worse to me, because my first interpretation/reading was if it progresses, it's ub, i.e. it may not (as in, shall not) progress instead of might not make progress

In D86233#2256648, @RalfJung wrote:

In D86233#2256563, @jdoerfert wrote:

In D86233#2256076, @RalfJung wrote:

Shouldn't it be "maynotprogress" (or "maybenoprogress" or so)? *Every* function *may* progress, but only those with this marker are also allowed to not "make progress".

The way to read it is:
Every function *mustmakeprogress* unless it has the *mayprogress* attribute.

So "may" is either they do or they don't, both are fine.
And, "must" means they have to or UB.

I understand. But this is not how the word "may" is usually used. When I say "you may do X", that typically implies that usually, X is not a thing you may do.
This can certainly be explained away, but the first impression people are going to have before reading the docs is likely going to be wrong.

In D84860#2182584, @fhahn wrote:

In D84860#2182575, @hfinkel wrote:

No, that's okay. The "based on" relationship flows through memory as well. The C spec for restrict does a better job than the LangRef does in this respect in formalizing that, but our semantics need to allow this too (otherwise our SSA conversation in SROA/mem2reg would not be sound for noalias pointers).

Agreed, thanks for confirming.

In this case, PTR is based on &x, and accesses through it in sideeffect are allowed even if x is noalias. One of the things that makes handling noalais pointers more-difficult than one might expect is, specifically, that we do need to check for cases where the restrict pointer itself escapes because then any other pointer of unknown provenance might be based on it.

I guess I am curious how the restrict pointer itself can escape in such a way?

What do you mean? Just like in your example, it can be assigned to a global variable (or, it can be assigned to *p, where p is an argument).

Oh right, I thought you meant there are cases where escaping is not allowed on the C level, but you meant we need to be careful during alias analysis?

In D84860#2182545, @fhahn wrote:

In D84860#2182487, @hfinkel wrote:

In D84860#2182432, @fhahn wrote:

In D84860#2182375, @hfinkel wrote:

We cannot remove the second load, because nothing guarantees that %x did not escape before calling the function. The new attribute provides exactly this guarantee, allowing us to remove the second load.

Why does noalias not provide the relevant guarantee? Even if the pointer had escaped, your call to @sideeffect would not be allowed to access the value through that escaped pointer (unless it were passed as an argument to @sideeffect) because that would violate the rule that all values accessed through a noalias pointer are accessed only through pointers based on the noalias pointer within the scope of the noalias pointer.

I think it boils down to the exact interpretation of noalias for the IR example below. Assume %0 would be marked as noalias. IIUC your comment correctly, @sideffect would not be allowed to modify the underlying object of %0 (this also matches my reading of the based-on rules, with value referring to LLVM IR/SSA value).

%struct.Foo = type { i8*, i8*, i32 }

@PTR = dso_local local_unnamed_addr global %struct.Foo* null, align 8

define dso_local i32 @test(%struct.Foo* %0) local_unnamed_addr #0 {
  %2 = getelementptr inbounds %struct.Foo, %struct.Foo* %0, i64 0, i32 2
  %3 = load i32, i32* %2, align 8, !tbaa !2
  store %struct.Foo* %0, %struct.Foo** @PTR, align 8, !tbaa !8
  tail call void bitcast (void (...)* @sideeffect to void ()*)() #2
  %4 = load i32, i32* %2, align 8, !tbaa !2
  %5 = add nsw i32 %4, %3
  ret i32 %5
}

declare dso_local void @sideeffect(...) local_unnamed_addr #1

If the above is true, then noalias would be too strict, because modifications via an escaped pointer in the function should be allowed with the uniqueptr semantics. IIUC we need to allow modifications, because references to passed-by-value objects can escape and used to modify the object in callees, as in something like the code below, where @sideeffect would be allowed to modify x via PTR

struct Foo *PTR;

int test(struct Foo x) {
    int a = x.i;
    PTR = &x;
    sideeffect();
    return x.i + a;
}

No, that's okay. The "based on" relationship flows through memory as well. The C spec for restrict does a better job than the LangRef does in this respect in formalizing that, but our semantics need to allow this too (otherwise our SSA conversation in SROA/mem2reg would not be sound for noalias pointers).

Agreed, thanks for confirming.

In this case, PTR is based on &x, and accesses through it in sideeffect are allowed even if x is noalias. One of the things that makes handling noalais pointers more-difficult than one might expect is, specifically, that we do need to check for cases where the restrict pointer itself escapes because then any other pointer of unknown provenance might be based on it.

I guess I am curious how the restrict pointer itself can escape in such a way?

In D84860#2182432, @fhahn wrote:

In D84860#2182375, @hfinkel wrote:

We cannot remove the second load, because nothing guarantees that %x did not escape before calling the function. The new attribute provides exactly this guarantee, allowing us to remove the second load.

Why does noalias not provide the relevant guarantee? Even if the pointer had escaped, your call to @sideeffect would not be allowed to access the value through that escaped pointer (unless it were passed as an argument to @sideeffect) because that would violate the rule that all values accessed through a noalias pointer are accessed only through pointers based on the noalias pointer within the scope of the noalias pointer.

I think it boils down to the exact interpretation of noalias for the IR example below. Assume %0 would be marked as noalias. IIUC your comment correctly, @sideffect would not be allowed to modify the underlying object of %0 (this also matches my reading of the based-on rules, with value referring to LLVM IR/SSA value).

%struct.Foo = type { i8*, i8*, i32 }

@PTR = dso_local local_unnamed_addr global %struct.Foo* null, align 8

define dso_local i32 @test(%struct.Foo* %0) local_unnamed_addr #0 {
  %2 = getelementptr inbounds %struct.Foo, %struct.Foo* %0, i64 0, i32 2
  %3 = load i32, i32* %2, align 8, !tbaa !2
  store %struct.Foo* %0, %struct.Foo** @PTR, align 8, !tbaa !8
  tail call void bitcast (void (...)* @sideeffect to void ()*)() #2
  %4 = load i32, i32* %2, align 8, !tbaa !2
  %5 = add nsw i32 %4, %3
  ret i32 %5
}

declare dso_local void @sideeffect(...) local_unnamed_addr #1

If the above is true, then noalias would be too strict, because modifications via an escaped pointer in the function should be allowed with the uniqueptr semantics. IIUC we need to allow modifications, because references to passed-by-value objects can escape and used to modify the object in callees, as in something like the code below, where @sideeffect would be allowed to modify x via PTR

struct Foo *PTR;

int test(struct Foo x) {
    int a = x.i;
    PTR = &x;
    sideeffect();
    return x.i + a;
}

We cannot remove the second load, because nothing guarantees that %x did not escape before calling the function. The new attribute provides exactly this guarantee, allowing us to remove the second load.

LGTM. Thank you.

Thanks for working on this. Please, however, take another pass of the test cases (especially those that are not in the PowerPC directory). Many of those should not be deleted, please change triple instead. They're testing general functionality.

LGTM

LGTM

In D81905#2129632, @kbobrovs wrote:

According to {{clang/include/clang/Basic/Builtins.def}}, you can represent pointers and references via * and &; for vectors - there are V, q, and E. I don't know how to deal with template parameter here, but reference to 4-component vector should be something like &E4i

Thanks. Somehow missed the '&' in the doc. I assume it should appear on the other side - "E4i&", though.
I still would like to understand what to do with C++ classes - do we forbid them in the "C++ intrinsics" or how do we represent them otherwise ("v&"?)

It's great that you're working on this. It's very important that we allow people to write code, structured and decomposed in a way that makes sense from an engineering and maintenance perspective, and have the compiler combine things later to avoid unnecessary overhead. This is just as much true for expressions of parallelism as it is for other aspects of the code.

LGTM

LGTM

Given that compile time looks okay (and it's clearly doing something, as its causing more simplification (or, at least, fewer instructions)), as does correctness, this LGTM. If we notice any significant performance problems, we can revert (but given that we're relatively close to branching, we should commit this sooner rather than later to give the maximum opportunity to spot such things).

In D82998#2138980, @dmgreen wrote:

It should be triggering relatively often, from looking at the tests/benchmarks I was seeing. Any code that uses pointer induction variables can trigger it. Whether that will common will be a matter of the style of the original code, and whether it actually proves move NoAlias will be variable. But it seems to come up enough to make code improvements in a fair number of cases.

From what I can tell it should be almost free though, in terms of compile time. Just an extra check of whether a phi is a simple recursion.

LGTM

Is getUserCost a better name than getInstructionCost? I wonder if we're moving in the wrong direction.

In D81905#2115449, @kbobrovs wrote:

Please upload the patch with full context.

Done. Sorry for missing that procedure detail.

In D81905#2113158, @kbobrovs wrote:

The patch by itself is just ItaniumDemangler.h interface enhancements (+ compilation error fix - objcProto->getProtocol()) :-)
If the change is OK by itself (not taking into account potential future uses), I would appreciate approval.

In D81905#2112803, @dexonsmith wrote:

In D81905#2105843, @kbobrovs wrote:

Yes, I infer semantics properties. I use demangler to implement lowering of "C++ intrinsics" - Itanium-mangled C++ name (e.g. templated function instantiation) - for my SPIRV-based target. Lowering depends on the name, template parameter values and argument types (if overloaded). https://github.com/intel/llvm/pull/1881

If its to infer some semantic property of the symbol then there probably are better ways of expressing that, like getting the frontend to add metadata or a function attribute or something.

It is not obvious to me that metadata + attributes is better than demangling (as long as it implemented correctly) - with demangling implementation is more self-encapsulated and simpler. But that's pretty much orthogonal to this code change.

Hmm, I would tend to agree with Erik here, that directly declaring the semantics via an attribute (or metadata) is better than encoding it in the name. I think it will be easier to reason about if you declare the semantics in the IR.

@hfinkel do you have thoughts?

LGTM too.

In D82005#2101927, @efriedma wrote:

I think this makes sense as a canonicalization: if we could express a select idiom using either a PHI or a select, the PHI probably makes sense as the canonical form. We clearly wouldn't want to canonicalize the other way. (The more general discussion of select vs. control flow is more centered around whether we should have the control flow in the first place; that's outside the scope of instcombine.)

The test coverage here seems a little slim; I'd like to see more coverage for interesting cases (in particular, cases where the PHI node doesn't have exactly two incoming edges).

We could try to generalize this transform to try to insert the PHI in blocks other than the block that contains the select; maybe leave that as a followup, though.

In D81728#2089713, @spatel wrote:

In D81728#2089644, @Flakebi wrote:

This combines instructions, so I think it belongs into the InstCombine pass. On the other hand, the f16 form of the intrinsics is not available on all targets, so this combination cannot be applied unconditionally but it needs to be gated depending on the target.

The fact that this pass recognizes target-specific intrinsics at all is widely regarded as a mistake:
http://lists.llvm.org/pipermail/llvm-dev/2016-July/102317.html

Target-specific transforms should look first at codegen combiners (SDAG or GlobalISel). If that's too late, consider a target-specific IR codegen pass (I think AMDGPU has a few examples of this already). If that's still too late, write a generic IR transform pass that accesses TTI?

I haven't seen a reason articulated as to why this change shouldn't be made that doesn't apply to the project as a whole;

An another thought: we might have a separate cmake flag to transform DIE -> unreachable, or some form of DIE into unreachable, so that we can test the performance difference easily going forward.

One more thought: We might split this into two patches. First, make the strings more informative. Then a patch for llvm_unreachable on top of that.

In D79507#2061428, @klausler wrote:

In D79507#2055514, @DavidTruby wrote:

The runtime behaviour when compiled with RELEASE is undefined. As I and @hfinkel mentioned, the point of that is optimisations that cannot otherwise be made by the compiler. I would say that runtime performance of release builds should be one of our main priorities...

Release builds of the compiler must have well-defined behavior, and that behavior must be to crash on a fatal internal error rather than to silently proceed to generate incorrect code, which is the worst thing a compiler can do to a user. Trading off protection against silent generation of bad code to gain some unmeasured performance improvement would be insanely irresponsible.

LGTM

I feel like there are a number of conflated issues here.

LGTM.

I don't think it is preferable to say the copy happens in the callee because the user could reasonably write something like this: https://godbolt.org/z/8vxz7P and we would need to resolve the discrepancy between the pure attribute and the byval passing in Clang.

This LGTM for consistency. Also, it reminds me that it's probably time for an RFC to remove QPX support in general.

In D77683#1973762, @dblaikie wrote:

In D77683#1973757, @jdoerfert wrote:

In D77683#1970826, @mehdi_amini wrote:

I am still not sure what "if someone has asked for extra review of a specific area" refers to?

As said earlier

If I understand this correctly, this is meant to cover situations where reviewers are active in an area and indicated an interest in reviewing basically everything.

Pretty much, yes.

this should mean that, if requested, all non-trivial patches should go through review. The current wording is very lenient, especially wrt. code owners.
While most people I talked to don't see owners as special per se (but just assume they have more responsibility), this paragraph says they have special rights.
Given the murky ways owners are "selected", I think we should have a well defined way to limit these rights without revoking the status.

I disagree here. I think it's suitable (within the way the LLVM project works) for code owners to commit without review - given they are the arbiters of what's acceptable in that subcomponent - ultimately they can veto anyone else (short of a broader project/code owner). Doesn't usually come to that, but that's what the ownership role means.

I don't think it's correct for arbitrary contributors to say "you need to/this component needs review-before-commit" - the code owner could say that if they really don't trust any of the contributors to conform to the direction they have in mind for the component (that's not to discredit the contributors - but that's the point of pre-commit review: to ensure it conforms to the code owners vision for the component).
privledges
(yes, code owners aren't meant to be dictators - but they're ultimately the final decider)

LGTM

LGTM

Is it difficult to create a test case for this?

LGTM

I don't object to doing this, but...

LGTM

Update formatting, etc., and add a reference in the new-contributor section of the dev policy in response to reviewer feedback.

In D71916#1904329, @baziotis wrote:

@hfinkel thanks a lot for this! (especially as a newcomer).
As you mentioned in llvm-dev, it might be a good idea to remove some "New Contributors" info from DeveloperPolicy and reference CodeReview.
In this case, I think it's good to put here the info about who commits what and what happens with the new contributors.

Updated to remove blank line and fix that (1 < N < M) should be (1 <= N <= M).

Updated per review feedback. Please let me know if you see anything else that should be addressed pre-commit.

I would like to thank everyone who provided feedback on this patch. I apologize for the slow turn-around time on this. I'll upload a revised version that, I believe, addresses all feedback.

In D75381#1899202, @davidxl wrote:

looks ok to me, but wait for Hal to have a final say.

Unfortunately, we cannot do this kind of thing just because it seems to make sense. The language semantics must be exactly satisfied by the IR-level semantics. I certainly agree that it would make sense for users to be able to mark invariant loads, but this mechanism simply might not be the right one.

In D73428#1879059, @jdoerfert wrote:

In D73428#1878746, @uenoku wrote:

And I have a question.

define i32 @f1(i32* %p, i32* %q){
entry:
  %0 = load i32, i32* %q
  %1 = load i32, i32* %p
  %add = add nsw i32 %1, %0
  ret i32 %add
}

define i32 @f2(i32* nocapture readonly %p) {
entry:
  %call = tail call i32 @f1(i32* %p, i32* %p)
  ret i32 %call
}

In this case, %call = tail call i32 @f1(i32* noalias %p, i32* noalias %p) is deduced because they have only "read-read" dependencies.
I agree that this is a sound deduction since there is no write. (I know Rust compiler is doing similar things)

However, looking back to the definition of noalias in LangRef, I think this annotation violates the definition. Is it allowed conventionally?

If this would violate the LangRef we would have a problem. I don't think it does but I think we need to make that explicit. I'll talk to @hfinkel and come back with lang ref patch or a revised version of this ;)

In D51664#1881963, @mkazantsev wrote:

WOW! Thanks!

LGTM

LGTM

LGTM

LGTM

In D73342#1841992, @sheredom wrote:

In D73342#1839002, @hfinkel wrote:

In D73342#1838953, @nikic wrote:

Can you explain in more detail why the current behavior is incorrect? Why is using the aliasing information in program order wrong? If the first load is noalias, then we should be able to deduplicate to that noalias load. If the second load is noalias, then generally we can't assume the first load is also noalias. Unless there is a must-exec relation between them, as @nlopes mentioned.

We have two issues here;

1. For any pass, if it changes any instruction with metadata that it does not understand, it should drop that metadata. That's the contract that we need to maintain.
2. For this AA metadata in particular, we need to intersect the metadata, and this is true to preserve semantics for almost all other known metadata (AA, profiling, fp-precision, debug) although likely for it is not correct to keep only one, or the other, because of potential dynamic dependencies on the results.

For AA, as an example, we can have something like this:

int * restrict r = a;
int x = should_alias ? *a : *r;

(essentially the same is true using casts as TBAA).

In any case, we should be calling the function llvm::combineMetadataForCSE here. Please update the patch to do that.

So when I use combineMetadataForCSE it breaks the invariant loads tests because the code there just says 'if one thing is invariant load and the other is not, throw the invariant load metadata away'.

What would be the correct fix for this? I don't know a ton about the intracacies of invariant loads.

In D73342#1839006, @nikic wrote:

In D73342#1838974, @jeroen.dobbelaere wrote:

Hal Finkel provided following example on the mailing list:

int * restrict r = a;
...
int x = noaliasing ? *r : *a;

This shows that the intersection of '*r' and '*a' must be taken.

So, I guess that the reasoning here is that noalias loads are speculatable and noalias violation is thus poison (not immediate UB), so we can't make inferences from one load to the other. Does that capture it?

In D73342#1838953, @nikic wrote:

Can you explain in more detail why the current behavior is incorrect? Why is using the aliasing information in program order wrong? If the first load is noalias, then we should be able to deduplicate to that noalias load. If the second load is noalias, then generally we can't assume the first load is also noalias. Unless there is a must-exec relation between them, as @nlopes mentioned.

LGTM

I'm not sure whether this is deliberate (but it seems weird) or just a bug. I can ask the GCC developers ...

Can we, at least, put this constant in a header file so we don't repeat it in several places? Also, can we write it as 0x20000000 so that it's more obvious what the value is.

In D71474#1816912, @simon_tatham wrote:

@nhaehnle, @hfinkel: anything left I need to do to this?

In D71407#1816911, @simon_tatham wrote:

@nhaehnle, @hfinkel: anything left I need to do to this?

In D72425#1811143, @andrew.w.kaylor wrote:

In D72425#1811098, @hfinkel wrote:

I like this general idea; couple of thoughts...

1. Clang has a similar system, and one disadvantage is that any time you change/add any message, it seems to trigger a large rebuild. Could we have this TG system generate separate .inc files for each category of things, so we don't have the same kind of rebuild problem.

Probably. One thing I was imagining coming out of the .td file(s) is an enum somewhere that assigns values to all of the messages. I suppose we could make that multiple enums, each with a fixed starting value to avoid triggering rebuilds. In general, these should lend themselves to logical groupings. We'd probably also want a way for downstream LLVM-based products to painlessly add their own remarks and groups should help with that.

2. Given that are arguments are named, can we use something like "Format string with optional specifier like %{NV}" instead of "Format string with optional specifier like %0"? I think that the former would be better.

Yes, that's a great idea. So, if I understand what you're suggesting, the entry for the remark I used as an example would look like this:

def remark_gvn_load_elim: OptRemark<"LoadElim", "load of type %Type eliminated", " in favor of %InfavorOfValue">;

Is that the idea?

I like this general idea; couple of thoughts...

In D70924#1804715, @RKSimon wrote:

@hfinkel Any more comments?

LGTM

Do you expect any effect at all from doing this? These are all scheduling barriers?

In D70758#1798961, @jsji wrote:

Ping ...

In D71954#1797765, @nemanjai wrote:

Add the feature to the BG cores as well.