Page MenuHomePhabricator

hfinkel (Hal Finkel)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 16 2012, 6:02 PM (403 w, 3 d)

Recent Activity

Wed, Jul 29

hfinkel added a comment to D84860: [LangRef] Add uniqueptr attribute..

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?

Wed, Jul 29, 10:31 AM · Restricted Project
hfinkel added a comment to D84860: [LangRef] Add uniqueptr attribute..

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?

Wed, Jul 29, 10:24 AM · Restricted Project
hfinkel added a comment to D84860: [LangRef] Add uniqueptr attribute..

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;
}
Wed, Jul 29, 10:00 AM · Restricted Project
hfinkel added a comment to D84860: [LangRef] Add uniqueptr attribute..

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.

Wed, Jul 29, 9:30 AM · Restricted Project

Mon, Jul 27

hfinkel accepted D83915: [PowerPC] Remove QPX/A2Q BGQ/BGP CNK support.

LGTM. Thank you.

Mon, Jul 27, 10:08 AM · Restricted Project, Restricted Project, Restricted Project

Thu, Jul 23

hfinkel added a comment to D83915: [PowerPC] Remove QPX/A2Q BGQ/BGP CNK support.

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.

Thu, Jul 23, 8:50 AM · Restricted Project, Restricted Project, Restricted Project

Tue, Jul 14

hfinkel added inline comments to D83635: [OpenMPOpt][WIP] Merge parallel regions.
Tue, Jul 14, 12:41 PM · Restricted Project
hfinkel added inline comments to D83423: [MC,NVPTX] Add MCAsmPrinter support for unsigned-only data directives..
Tue, Jul 14, 12:35 PM · Restricted Project
hfinkel added inline comments to D83423: [MC,NVPTX] Add MCAsmPrinter support for unsigned-only data directives..
Tue, Jul 14, 12:26 PM · Restricted Project
hfinkel added inline comments to D83423: [MC,NVPTX] Add MCAsmPrinter support for unsigned-only data directives..
Tue, Jul 14, 12:15 PM · Restricted Project
hfinkel accepted D55290: [docs] Update llvm.loop metadata documentation..

LGTM

Tue, Jul 14, 8:44 AM · Restricted Project
hfinkel accepted D83781: [LangRef] Fix condition for when a loop is considered parallel..

LGTM

Tue, Jul 14, 8:36 AM · Restricted Project
hfinkel added a comment to D81905: Enhance Itanium demangler interface..

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&"?)

Tue, Jul 14, 7:05 AM · Restricted Project

Mon, Jul 13

hfinkel added inline comments to D83423: [MC,NVPTX] Add MCAsmPrinter support for unsigned-only data directives..
Mon, Jul 13, 7:29 PM · Restricted Project
hfinkel added inline comments to D83423: [MC,NVPTX] Add MCAsmPrinter support for unsigned-only data directives..
Mon, Jul 13, 6:27 PM · Restricted Project

Sun, Jul 12

hfinkel added a comment to D83635: [OpenMPOpt][WIP] Merge parallel regions.

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.

Sun, Jul 12, 11:43 AM · Restricted Project

Jul 11 2020

hfinkel accepted D83576: [BasicAA] Fix -basicaa-recphi for geps with negative offsets.

LGTM

Jul 11 2020, 12:31 PM · Restricted Project

Jul 10 2020

hfinkel added inline comments to D70326: [docs] LLVM Security Group and Process.
Jul 10 2020, 6:48 PM · Restricted Project
hfinkel accepted D83590: [PowerPC][MachinePipeliner] Enable pipeliner if hasInstrSchedModel.

LGTM

Jul 10 2020, 6:24 PM · Restricted Project
hfinkel added inline comments to D83576: [BasicAA] Fix -basicaa-recphi for geps with negative offsets.
Jul 10 2020, 2:00 PM · Restricted Project

Jul 9 2020

hfinkel accepted D82998: [BasicAA] Enable -basic-aa-recphi by default.

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

Jul 9 2020, 4:31 AM · Restricted Project

Jul 8 2020

hfinkel added a comment to D82998: [BasicAA] Enable -basic-aa-recphi by default.

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.

Jul 8 2020, 1:05 PM · Restricted Project

Jul 2 2020

hfinkel accepted D81159: CodeGenPrep: remove AssertingVH references before deleting dead instructions..

LGTM

Jul 2 2020, 9:10 AM · Restricted Project

Jul 1 2020

hfinkel added a comment to D79483: [CostModel] Replace getUserCost with getInstructionCost..

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

Jul 1 2020, 8:05 AM · Restricted Project

Jun 26 2020

hfinkel added a comment to D81905: Enhance Itanium demangler interface..

Please upload the patch with full context.

Done. Sorry for missing that procedure detail.

Jun 26 2020, 3:39 PM · Restricted Project

Jun 25 2020

hfinkel added a comment to D81905: Enhance Itanium demangler interface..

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.

Jun 25 2020, 12:28 PM · Restricted Project

Jun 24 2020

hfinkel added a comment to D81905: Enhance Itanium demangler interface..

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?

Jun 24 2020, 4:20 PM · Restricted Project

Jun 20 2020

hfinkel accepted D82261: [ValueTracking, BasicAA] Don't simplify instructions.

LGTM too.

Jun 20 2020, 4:25 PM · Restricted Project

Jun 18 2020

hfinkel added a comment to D82005: [InstCombine] Replace selects with Phis.

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.

Jun 18 2020, 7:08 PM · Restricted Project

Jun 16 2020

hfinkel added inline comments to D80865: [TableGen] defm in a loop is not final (bug fix).
Jun 16 2020, 9:51 PM · Restricted Project

Jun 12 2020

hfinkel added a comment to D81728: [InstCombine] Add target-specific inst combining.

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?

Jun 12 2020, 2:14 PM · Restricted Project, Restricted Project, Restricted Project

Jun 1 2020

hfinkel added a comment to D79507: [flang] Change DIE("unreachable") cases to use llvm_unreachable.

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;

Jun 1 2020, 3:11 PM · Restricted Project, Restricted Project
hfinkel added a comment to D79507: [flang] Change DIE("unreachable") cases to use llvm_unreachable.

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.

Jun 1 2020, 9:06 AM · Restricted Project, Restricted Project
hfinkel added a comment to D79507: [flang] Change DIE("unreachable") cases to use llvm_unreachable.

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.

Jun 1 2020, 9:06 AM · Restricted Project, Restricted Project
hfinkel added a comment to D79507: [flang] Change DIE("unreachable") cases to use llvm_unreachable.

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.

Jun 1 2020, 8:34 AM · Restricted Project, Restricted Project

May 30 2020

hfinkel created D80865: [TableGen] defm in a loop is not final (bug fix).
May 30 2020, 5:48 AM · Restricted Project

May 26 2020

hfinkel accepted D79991: [TableGen] Fix non-standard escape warnings for braces in InstAlias.

LGTM

May 26 2020, 10:49 AM · Restricted Project
hfinkel added a comment to D79507: [flang] Change DIE("unreachable") cases to use llvm_unreachable.

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

May 26 2020, 10:17 AM · Restricted Project, Restricted Project

May 19 2020

hfinkel added inline comments to D79991: [TableGen] Fix non-standard escape warnings for braces in InstAlias.
May 19 2020, 5:54 AM · Restricted Project
hfinkel added inline comments to D79991: [TableGen] Fix non-standard escape warnings for braces in InstAlias.
May 19 2020, 4:17 AM · Restricted Project

May 6 2020

hfinkel added inline comments to D79359: OpenMPOpt Remarks Support.
May 6 2020, 4:30 PM · Restricted Project, Restricted Project
hfinkel accepted D72749: [PowerPC] Add exception constraint to FP sqrt, fma, min/max.

LGTM.

May 6 2020, 3:55 PM · Restricted Project
hfinkel added a comment to D79454: [IR] `byval` arguments cause reads at call sites.

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.

May 6 2020, 10:46 AM · Restricted Project

Apr 29 2020

hfinkel accepted D78986: [PowerPC-QPX] adjust operands order of qpx vector fma instructions..

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

Apr 29 2020, 9:39 AM · Restricted Project

Apr 13 2020

hfinkel added a comment to D77683: [Docs] Make code review policy clearer about requested pre-commit reviews.

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)

Apr 13 2020, 10:12 AM · Restricted Project
hfinkel accepted D77975: [CallGraphUpdater] Properly remove strongly connected components (old PM).

LGTM

Apr 13 2020, 9:39 AM · Restricted Project

Apr 12 2020

hfinkel accepted D77854: [CallGraphUpdater] Update the ExternalCallingNode for node replacements.

LGTM

Apr 12 2020, 10:40 AM · Restricted Project
hfinkel added a comment to D77854: [CallGraphUpdater] Update the ExternalCallingNode for node replacements.

Is it difficult to create a test case for this?

Apr 12 2020, 8:00 AM · Restricted Project
hfinkel accepted D77855: [CallGraphUpdater] Remove nodes from their SCC (old PM).

LGTM

Apr 12 2020, 8:00 AM · Restricted Project

Mar 27 2020

hfinkel added a comment to D76886: [InlineFunction] Disable emission of alignment assumptions by default.

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

Mar 27 2020, 10:55 AM · Restricted Project

Mar 26 2020

hfinkel accepted D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG").

LGTM

Mar 26 2020, 5:25 PM · Restricted Project

Mar 25 2020

hfinkel added inline comments to D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG").
Mar 25 2020, 11:53 AM · Restricted Project
hfinkel added inline comments to D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG").
Mar 25 2020, 8:04 AM · Restricted Project

Mar 24 2020

hfinkel added inline comments to D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG").
Mar 24 2020, 3:39 PM · Restricted Project

Mar 6 2020

hfinkel committed rGfa913f8980de: Add the CodeReview Documentation to GettingInvolved TOC (authored by hfinkel).
Add the CodeReview Documentation to GettingInvolved TOC
Mar 6 2020, 9:27 PM
hfinkel committed rG4d0339aecb60: High-Level Code-Review Documentation Update (authored by hfinkel).
High-Level Code-Review Documentation Update
Mar 6 2020, 8:23 PM
hfinkel closed D71916: High-Level Code-Review Documentation Update.
Mar 6 2020, 8:23 PM · Restricted Project

Mar 4 2020

hfinkel updated the diff for D71916: High-Level Code-Review Documentation Update.

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

Mar 4 2020, 10:18 PM · Restricted Project
hfinkel added a comment to D71916: High-Level Code-Review Documentation Update.

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

Mar 4 2020, 10:18 PM · Restricted Project

Feb 29 2020

hfinkel updated the diff for D71916: High-Level Code-Review Documentation Update.

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

Feb 29 2020, 1:06 PM · Restricted Project
hfinkel added inline comments to D71916: High-Level Code-Review Documentation Update.
Feb 29 2020, 1:06 PM · Restricted Project

Feb 28 2020

hfinkel updated the diff for D71916: High-Level Code-Review Documentation Update.

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

Feb 28 2020, 9:10 PM · Restricted Project
hfinkel added a comment to D71916: High-Level Code-Review Documentation Update.

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.

Feb 28 2020, 9:06 PM · Restricted Project
hfinkel added inline comments to D73342: Fix EarlyCSE to intersect aliasing metadata..
Feb 28 2020, 7:35 PM · Restricted Project
hfinkel accepted D75381: [WinEH] Fix inttoptr+phi optimization in presence of catchswitch.

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

Feb 28 2020, 6:56 PM · Restricted Project
hfinkel added inline comments to D75381: [WinEH] Fix inttoptr+phi optimization in presence of catchswitch.
Feb 28 2020, 2:09 PM · Restricted Project

Feb 27 2020

hfinkel requested changes to D75285: Mark restrict pointer or reference to const as invariant.

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.

Feb 27 2020, 12:23 PM

Feb 19 2020

hfinkel added a comment to D73428: [Attributor] Improve `noalias` deduction based on memory information.

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

Feb 19 2020, 4:17 PM · Restricted Project
hfinkel added a comment to D51664: [IR] Lazily number instructions for local dominance queries.

WOW! Thanks!

Feb 19 2020, 12:22 AM · Restricted Project

Feb 18 2020

hfinkel added inline comments to D73342: Fix EarlyCSE to intersect aliasing metadata..
Feb 18 2020, 8:53 AM · Restricted Project

Feb 11 2020

hfinkel accepted D74353: [BasicAA] Make BasicAA a cfg pass..

LGTM

Feb 11 2020, 12:59 AM · Restricted Project

Feb 7 2020

hfinkel accepted D74220: [TableGen] Fix spurious type error in bit assignment..

LGTM

Feb 7 2020, 6:57 AM · Restricted Project

Feb 4 2020

hfinkel accepted D70927: Introduce a CallGraph updater helper class.

LGTM

Feb 4 2020, 7:27 AM · Restricted Project

Jan 31 2020

hfinkel added inline comments to D70927: Introduce a CallGraph updater helper class.
Jan 31 2020, 12:53 PM · Restricted Project
hfinkel accepted D73528: [Inliner][NoAlias] Use call site attributes too.

LGTM

Jan 31 2020, 12:53 PM · Restricted Project

Jan 27 2020

hfinkel added a comment to D73342: Fix EarlyCSE to intersect aliasing metadata..

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.

Jan 27 2020, 12:33 PM · Restricted Project

Jan 24 2020

hfinkel added a comment to D73342: Fix EarlyCSE to intersect aliasing metadata..

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?

Jan 24 2020, 9:46 AM · Restricted Project
hfinkel added a comment to D73342: Fix EarlyCSE to intersect aliasing metadata..

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.

Jan 24 2020, 8:58 AM · Restricted Project

Jan 21 2020

hfinkel accepted D73154: [AliasAnalysis] Add missing FMRB_* enums..

LGTM

Jan 21 2020, 6:46 PM · Restricted Project
hfinkel added inline comments to D72996: [Sema] Attempt to perform call-size-specific `__attribute__((alloc_align(param_idx)))` validation.
Jan 21 2020, 6:29 PM · Restricted Project
hfinkel added inline comments to D72996: [Sema] Attempt to perform call-size-specific `__attribute__((alloc_align(param_idx)))` validation.
Jan 21 2020, 4:04 PM · Restricted Project

Jan 20 2020

hfinkel added a comment to D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction.

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

Jan 20 2020, 10:26 AM · Restricted Project

Jan 19 2020

hfinkel added a comment to D72998: [IR] Attribute/AttrBuilder: use Value::MaximumAlignment magic constant.

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.

Jan 19 2020, 8:49 AM · Restricted Project, Restricted Project

Jan 13 2020

hfinkel accepted D71474: [TableGen] Introduce an if/then/else statement..

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

Jan 13 2020, 8:04 PM · Restricted Project
hfinkel added inline comments to D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction.
Jan 13 2020, 8:04 PM · Restricted Project
hfinkel accepted D71407: [TableGen] Introduce a `defvar` statement..

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

Jan 13 2020, 8:04 PM · Restricted Project

Jan 8 2020

hfinkel added a comment to D72425: [OptRemark] RFC: Introduce a message table for OptRemarks.

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.

  1. 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?

Jan 8 2020, 5:10 PM · Restricted Project
hfinkel added a comment to D72425: [OptRemark] RFC: Introduce a message table for OptRemarks.

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

Jan 8 2020, 4:24 PM · Restricted Project

Jan 5 2020

hfinkel accepted D70924: [Metadata] Add TBAA struct metadata to `AAMDNode`.

@hfinkel Any more comments?

Jan 5 2020, 3:19 PM · Restricted Project

Dec 31 2019

hfinkel accepted D72038: [PowerPC] Handle constant zero bits in BitPermutationSelector.

LGTM

Dec 31 2019, 6:20 PM · Restricted Project

Dec 30 2019

hfinkel added a comment to D71983: [PowerPC] Set the SideEffects of branch & call instructions from 1 to 0.

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

Dec 30 2019, 3:53 PM · Restricted Project
hfinkel accepted D70758: [PowerPC][NFC] Rename record instructions to use _rec suffix instead of o.

Ping ...

Dec 30 2019, 3:53 PM · Restricted Project

Dec 28 2019

hfinkel accepted D71954: [PowerPC] Change default for unaligned FP access for older subtargets.

Add the feature to the BG cores as well.

Dec 28 2019, 8:53 AM · Restricted Project

Dec 27 2019

hfinkel added inline comments to D70927: Introduce a CallGraph updater helper class.
Dec 27 2019, 12:24 AM · Restricted Project
hfinkel added inline comments to D71916: High-Level Code-Review Documentation Update.
Dec 27 2019, 12:15 AM · Restricted Project

Dec 26 2019

hfinkel created D71916: High-Level Code-Review Documentation Update.
Dec 26 2019, 2:43 PM · Restricted Project

Dec 18 2019

hfinkel accepted D70767: [Attributor] Add an Attributor CG-SCC pass.

LGTM

Dec 18 2019, 5:18 AM · Restricted Project

Dec 17 2019

hfinkel added a comment to D71241: [OpenMP][WIP] Use overload centric declare variants.

Most probably, we can use this solution without adding a new expression. DeclRefExpr class can contain 2 decls: FoundDecl and the Decl being used. You can use FoundDecl to point to the original function and used decl to point to the function being called in this context. But at first, we need to be sure that we can handle all corner cases correctly.

What new expression are you talking about?

To be clear, I believe he's talking about the new expression that I proposed we add in order to represent this kind of call. If that's not needed, and we can use the FoundDecl/Decl pair for that purpose, that should also be considered.

This solution already does point to both declarations, as shown here: https://reviews.llvm.org/D71241#1782504

Exactly. We need to check if the MemberRefExpr can do this too to handle member functions correctly.
And need to wait for opinion from Richard Smith about the design. We need to be sure that it won't break compatibility with C/C++ in some corner cases. Design bugs are very hard to solve and I want to be sure that we don't miss anything. And we provide full compatibility with both C and C++.

Dec 17 2019, 4:16 AM · Restricted Project

Dec 16 2019

hfinkel added a comment to D71241: [OpenMP][WIP] Use overload centric declare variants.

Most probably, we can use this solution without adding a new expression. DeclRefExpr class can contain 2 decls: FoundDecl and the Decl being used. You can use FoundDecl to point to the original function and used decl to point to the function being called in this context. But at first, we need to be sure that we can handle all corner cases correctly.

What new expression are you talking about?

Dec 16 2019, 10:08 PM · Restricted Project

Dec 13 2019

hfinkel accepted D67923: [TLI] Support for per-Function TLI that overrides available libfuncs.

Please take a look. This is now updated to reflect the commit of D71193, which translated the options to the new attributes. I also removed some comments that I realized didn't make sense, as we need to keep a baseline availability array on the base TLII since that is set based on the architecture. We simply override this for no-builtin* attributes on the function. I removed the code from clang that was setting up the availability array based on the options, and all tests pass.

Dec 13 2019, 6:01 PM · Restricted Project, Restricted Project