hfinkel (Hal Finkel)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 16 2012, 6:02 PM (235 w, 4 d)

Recent Activity

Yesterday

hfinkel added inline comments to D33328: [CodeGen] Pessimize aliasing for union members (and may-alias) objects.
Tue, May 23, 9:32 AM
hfinkel added inline comments to D33328: [CodeGen] Pessimize aliasing for union members (and may-alias) objects.
Tue, May 23, 9:25 AM
hfinkel accepted D33402: [RuntimeDyld, PowerPC] Fix check for external symbols when detecting reloction overflow.

LGTM, thanks!

Tue, May 23, 6:53 AM

Mon, May 22

hfinkel added inline comments to D31772: [PowerPC] Add pass to expand extra memcpy calls.
Mon, May 22, 3:35 PM
hfinkel added inline comments to D33396: [LV] Report multiple reasons for not vectorizing under allowExtraAnalysis.
Mon, May 22, 3:09 PM
hfinkel updated the diff for D31239: [WIP] Add Caching of Known Bits in InstCombine.

Rebased patch provided by Craig Topper.

Mon, May 22, 3:02 PM
hfinkel accepted D33396: [LV] Report multiple reasons for not vectorizing under allowExtraAnalysis.

LGTM

Mon, May 22, 1:58 PM
hfinkel added a comment to D33402: [RuntimeDyld, PowerPC] Fix check for external symbols when detecting reloction overflow.

Also, can we have a test case for this?

Mon, May 22, 7:33 AM
hfinkel accepted D33403: [RuntimeDyld, PowerPC] Fix relocation detection overflow.

LGTM

Mon, May 22, 7:33 AM
hfinkel accepted D33402: [RuntimeDyld, PowerPC] Fix check for external symbols when detecting reloction overflow.

LGTM

Mon, May 22, 7:30 AM

Sun, May 21

hfinkel added a comment to D32737: [Constants][SVE] Represent the runtime length of a scalable vector.

I've only lightly read the spec, but it looks like the vector length can be controlled by writing to the ZCR_ELn registers (so, e.g. user code could make a syscall to change the vector length)?

As I explained to Hal on his comment, that is correct but doesn't have the effect you're expecting.

Sun, May 21, 12:36 PM
hfinkel added a comment to D32782: Add pthread_self function prototype and make it speculatable..

OK, I think I found out the cause. I guess this patch was wrong, my bad.
GCC doesn't do anything special with pthread_self per-se.
What GCC does is speculating the glibc implementation of pthread_self is declared with __attribute__(const).
The semantic of the attribute is that of "The const attribute is specified as asserting that the function does not examine any data except the arguments. " [1]
If the function has no arguments, it has to return the same value every time.

Therefore, it speculates.
I think that other libc implementation are free to not declare pthread_self with that attribute. In fact, from what I can see, the FreeBSD version doesn't use that argument.
In other words, I don't think we're allowed to do anything with pthread_self() in general as POSIX specifies weaks guarantees.

So there a general implication we can implement: __attribute__(const). + zero arguments == speculatable?

I'm not sure if that's the exact recipe, but yes, it seems to be on the right path.

Also, I fail to see how it would not be safe to tread pthread_self as speculatable? Same for getpid.

The standard says that the call always succeeds and always returns the thread id. The thread ids are opaque, and I can imagine there being multiple "self" values (pthread_equal would just return true for all of them), thus making pthread_self non-const. However, I can think of no reason why an implementation would do this, don't know of any that do, the behavior would only be observable via some non-standard interface, and I'm happy to cross that bridge if we come to it.

I don't understand the bit about getpid(). In that case forking actually could change the value and you might end up in trouble if you rely on that to write temporary directories (as it's generally done).

Oh. You're right. Also, that seems to also rule out this as well. fork() could also change the value of pthread_self() I'd imagine.

It's slightly different, at least in my opinion.
getpid() returns a pid_t.

pid_t is defined to be an integer type, although POSIX doesn't put any restrictions on the size.
http://pubs.opengroup.org/onlinepubs/009696699/basedefs/sys/types.h.html

glibc (and FreeBSD libc) decide to make it an int. People know it's an integer and use as such.

On the other hand, pthread_t is considered to be an opaque type. It can be an integer/a struct/you name it.
In fact, this completely opaque implementation detail wildly varies across implementations (for LinuxThreads, it's an integer, for NPTL, a pointer).
http://man7.org/linux/man-pages/man3/pthread_self.3.html
Linux also documents that using pthread_t in anything that's not pthread calls results in an unspecified behaviour, while pid_t can be used freely.

So, it's not quite the same, but it's still debatable whether the transformation should be performed or not.

Sun, May 21, 1:22 AM

Sat, May 20

hfinkel added a comment to D32782: Add pthread_self function prototype and make it speculatable..

OK, I think I found out the cause. I guess this patch was wrong, my bad.
GCC doesn't do anything special with pthread_self per-se.
What GCC does is speculating the glibc implementation of pthread_self is declared with __attribute__(const).
The semantic of the attribute is that of "The const attribute is specified as asserting that the function does not examine any data except the arguments. " [1]
If the function has no arguments, it has to return the same value every time.

Therefore, it speculates.
I think that other libc implementation are free to not declare pthread_self with that attribute. In fact, from what I can see, the FreeBSD version doesn't use that argument.
In other words, I don't think we're allowed to do anything with pthread_self() in general as POSIX specifies weaks guarantees.

So there a general implication we can implement: __attribute__(const). + zero arguments == speculatable?

I'm not sure if that's the exact recipe, but yes, it seems to be on the right path.

Also, I fail to see how it would not be safe to tread pthread_self as speculatable? Same for getpid.

The standard says that the call always succeeds and always returns the thread id. The thread ids are opaque, and I can imagine there being multiple "self" values (pthread_equal would just return true for all of them), thus making pthread_self non-const. However, I can think of no reason why an implementation would do this, don't know of any that do, the behavior would only be observable via some non-standard interface, and I'm happy to cross that bridge if we come to it.

I don't understand the bit about getpid(). In that case forking actually could change the value and you might end up in trouble if you rely on that to write temporary directories (as it's generally done).

Sat, May 20, 5:54 PM
hfinkel added a comment to D32782: Add pthread_self function prototype and make it speculatable..

OK, I think I found out the cause. I guess this patch was wrong, my bad.
GCC doesn't do anything special with pthread_self per-se.
What GCC does is speculating the glibc implementation of pthread_self is declared with __attribute__(const).
The semantic of the attribute is that of "The const attribute is specified as asserting that the function does not examine any data except the arguments. " [1]
If the function has no arguments, it has to return the same value every time.

Therefore, it speculates.
I think that other libc implementation are free to not declare pthread_self with that attribute. In fact, from what I can see, the FreeBSD version doesn't use that argument.
In other words, I don't think we're allowed to do anything with pthread_self() in general as POSIX specifies weaks guarantees.

Sat, May 20, 5:44 PM
hfinkel added a comment to D32737: [Constants][SVE] Represent the runtime length of a scalable vector.

What are the semantics of select when the two vectors have different width? Does store do a memory allocation?

Maybe I misunderstood, but won't those selects be ill-typed?

Sat, May 20, 5:15 PM

Tue, May 16

hfinkel accepted D33017: [PPC] Properly update register save area offsets.

This passes a double bootstrap with full testing. So I think it's fine.

Tue, May 16, 11:04 AM

Sat, May 13

hfinkel added a comment to D18738: Add new !unconditionally_dereferenceable load instruction metadata.

@hfinkel Oh, my bad--I now remember that this came up long ago...

...

@sanjoy Can you confirm that a dereferenceable attribute on getelementptr would be an acceptable IR extension?

A GEP that always produces a dereferenceable value may be tricky to implement since we'll have to remember to strip said attribute whenever we hoist GEPs; and LLVM likes to hoist GEP's without thinking too much. But I believe this is going in the right direction -- we should not have soundness problems as long as the safety of some operation is guaranteed by some other preceding operation.

Sat, May 13, 11:56 PM

Fri, May 12

hfinkel added a comment to D33136: [ValueTracking] Don't let isAddOfNonZero look at adds with a PHI node as input.

Can we add a mode (e.g. some extra boolean parameter) to ValueTracking to enable this kind of restriction? We could then enable this mode when in BasicAA. I don't want to, in general, disable things here to work around the fact that BasicAA is doing some unsound.

Fri, May 12, 9:40 AM

Thu, May 11

hfinkel added a comment to D18738: Add new !unconditionally_dereferenceable load instruction metadata.

@sanjoy

I'd be okay (even happy! :) ) if you add a @llvm.safe.load.<ty> intrinsic that never has UB, and returns undef if the address passed to it is not dereferenceable. That intrinsic could then be marked speculatable. If needed, we could even implement the intrinsic by trying to read from the address passed in, and by catching the SIGSEGV or SIGBUS, if any.

First, it is not realistically possible to implement on most platforms (SEH *might* be fine but even then I'm not sure). Second, every pass that looks at loads will have to be amended in an invasive way (a quick look at LICM alone tells me this will be a nightmare as it passes bare LoadInst* everywhere...). Third, it would be crippled compared to real loads, as it won't support some attributes loads do (e.g. !invariant.load) and adding support for that will, AFAICT, require adding a new return value attribute, similar to how nonnull and dereferenceable are currently implemented there. Fourth, I don't think it will be easy to plug into the current AA architecture.

Even if all the rest was fixed, the lack of !invariant.load alone makes it completely useless for our use case so I suggest not discussing this proposal further.

Let's back away a bit. My current issue is that my frontend generates lots of deeply nested loads in inner loops. This happens because it is translating Python, and you can easily end up with something like:

for bs in as:
  for b in bs:
    c += self.core.dds0.ftw

The frontend guarantees that all these pointers are always dereferenceable. In fact every single SSA value of pointer type in the entire emitted IR is dereferenceable and nonnull. The frontend also knows that most of these loads are ultimately constant (in the case above, a simplified extract of real-world code, self.core.dds0 will never change for the entire program lifetime). The frontend is not able to hoist the loads into preheader itself because it does not perform inlining and so does not have enough visibility.

How can I tell LLVM that these loads may be safely hoisted?

Thu, May 11, 1:19 PM

Tue, May 9

hfinkel added inline comments to D29654: [OpenMP] Integrate OpenMP target region cubin into host binary.
Tue, May 9, 7:48 PM
hfinkel added inline comments to D32888: TableGen: Add support of Intrinsics with multiple returns.
Tue, May 9, 7:32 PM

Mon, May 8

hfinkel added a comment to D32006: Mark invariant.group.barrier as inaccessiblememonly.

Is it possible to add "writeonly"? I am not sure if it will help in any way, but the tests seems to be working it.

Mon, May 8, 8:13 AM

Sun, May 7

hfinkel added inline comments to D32059: Make AssumptionCache's interface return a range of Values.
Sun, May 7, 6:52 PM
hfinkel added inline comments to D32006: Mark invariant.group.barrier as inaccessiblememonly.
Sun, May 7, 2:17 PM

Thu, May 4

hfinkel added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

Could we check this in the verifier? One advantage this might have is that, for blocks ending in unreachable, we can easily realize that we don't need the restoration code.

Are we adding restoration code to such blocks now? That could be checked in the verifier, but it's not incorrect. Maybe a warning could be issued...

Thu, May 4, 7:54 AM
hfinkel added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

Yeah I like that way of modeling things explicitely. Unfortunately that is not how most llvm targets work today.

Would it be reasonable to implement that for all targets?

Thu, May 4, 7:35 AM

Fri, Apr 28

hfinkel added a comment to D20116: Add speculatable function attribute.

Okay, unfortunately, this is only useful to me if we allow it on function declarations

I had somehow missed this bit ^ and I was under the impression that the main motivation for a general attribute was more completeness than anything else.

Fri, Apr 28, 1:11 PM
hfinkel added a comment to D20116: Add speculatable function attribute.

Why only for intrinsics? I thought we had concluded that we'd only allow it for declarations and not on call sites (which may technically mean on call sited but only matching the declaration). I think it is important that we can apply it to regular functions.

I think a general speculatable attribute that is allowed only on functions decls is *less problematic*[0] that a context sensitive one, but I think speculatable intrinsics are clearly okay. Therefore my opinion is (which I expressed on IRC to Matt) is to first land the intrinsic variant of this, since that's what he's blocked on; and then we can go ahead with more aggressive variants on subsequent patches.

[0] https://reviews.llvm.org/D20116#709352

Fri, Apr 28, 12:41 PM
hfinkel added a comment to D20116: Add speculatable function attribute.

Only allow for intrinsics

Why only for intrinsics? I thought we had concluded that we'd only allow it for declarations and not on call sites (which may technically mean on call sited but only matching the declaration). I think it is important that we can apply it to regular functions.

I think so too, but @sanjoy said to restrict it to intrinsics for now.

Fri, Apr 28, 12:31 PM
hfinkel added a comment to D20116: Add speculatable function attribute.

Only allow for intrinsics

Fri, Apr 28, 12:13 PM

Thu, Apr 27

hfinkel committed rL301590: Add references to past LLVM in HPC workshops.
Add references to past LLVM in HPC workshops
Thu, Apr 27, 2:07 PM
hfinkel added a comment to D32605: Recognize CTLZ builtin.

We could add a TTI callback like we have for popcnt. You could also argue that this is the better canonical form because it can enable other analysis/optimizations (and we should fix the expansion if it is suboptimal).

Thu, Apr 27, 12:19 PM
hfinkel added a comment to D32582: [InstCombine] Add range metadata to cttz/ctlz/ctpop intrinsic calls based on known bits.

What analysis benefits from this range data? Should we teach that analysis to use known-bits information directly instead of using metadata as an analysis cache?

Thu, Apr 27, 4:15 AM

Mon, Apr 24

hfinkel added inline comments to D32433: Compute safety information in a much finer granularity..
Mon, Apr 24, 3:39 PM
hfinkel accepted D32433: Compute safety information in a much finer granularity..

LGTM

Mon, Apr 24, 9:09 AM

Apr 22 2017

hfinkel added inline comments to D32387: [PartialInlining] Add Optimization Remark Support.
Apr 22 2017, 7:48 PM

Apr 21 2017

hfinkel updated the diff for D32197: [TySan] A Type Sanitizer (Runtime Library).

Mark the types of globals in a generated ctor-called function (added a runtime test for this). memcpy/memmove should do the corresponding thing for the type shadow data.

Apr 21 2017, 5:21 PM
hfinkel updated the diff for D32198: [TySan] A Type Sanitizer (LLVM).

Mark the types of globals in a generated ctor-called function. memcpy/memmove should do the corresponding thing for the type shadow data.

Apr 21 2017, 5:18 PM
hfinkel updated the diff for D32199: [TySan] A Type Sanitizer (Clang).

Output metadata to provide the types of globals (similar to how Clang marks globals for asan).

Apr 21 2017, 5:16 PM
hfinkel updated the diff for D32198: [TySan] A Type Sanitizer (LLVM).

Rename TBAASanitizer -> TypeSanitizer

Apr 21 2017, 7:21 AM
hfinkel updated the diff for D32197: [TySan] A Type Sanitizer (Runtime Library).

Rename TBAASanitizer -> TypeSanitizer

Apr 21 2017, 7:19 AM
hfinkel updated the diff for D32199: [TySan] A Type Sanitizer (Clang).

Rename TBAASanitizer -> TypeSanitizer

Apr 21 2017, 7:18 AM
hfinkel added a comment to D32199: [TySan] A Type Sanitizer (Clang).
  1. C's "effective type" rule allows writes to set the type pretty much unconditionally, unless the storage is for a variable with a declared type

To come back to this point: We don't really implement these rules now, and it is not clear that we will. The problem here is that, if we take the specification literally, then we can't use our current TBAA at all. The problem is that if we have:

write x, !tbaa "int"
read x, !tbaa "int"
write x, !tbaa "float"

TBAA will currently tell us that the "float" write aliases with neither the preceding read nor the preceding write.

Right, C's TBAA rules do not (in general) permit a store to be reordered before a memory operation of a different type, they only allow loads to be moved before stores. (Put another way, they do not tell you that pointers point to distinct memory locations, just that a stored value cannot be observed by a load of a different type.) You get the more general "distinct memory locations" result only for objects of a declared type.

C++ is similar, except that (because object lifetimes do not currently begin magically due to a store) you /can/ reorder stores past a memory operation of a different type if you know no object's lifetime began in between. (But currently we do not record all lifetime events in IR, so we can't do that today. Also, we may be about to lose the property that you can statically determine a small number of places that might start an object lifetime.)

Also, a strict reading of C's access rules seems to rule out the premise underlying our struct-path TBAA entirely. So long as I'm accessing a value using a struct that has some member, including recursively, with that type, then it's fine. The matching of the relative offsets is a sufficient, but not necessary, condition for well-defined access. C++ has essentially the same language (and, thus, potentially the same problem).

I agree this rule is garbage, but it's not as permissive as I think you're suggesting. The rule says that you can use an lvalue of struct type to access memory of struct field type. In C this happens during struct assignment, for instance. It does *not* permit using an lvalue of struct field type to access unrelated fields of the same struct. So C appears to allow this nonsense:

char *p = malloc(8);
*(int*)p = 0;
*(int*)(p + 4) = 0;
struct S {int n; float f;} s = *(struct S*)p; // use lvalue of type `struct S` to access object of effective type `int`, to initialize a `float`

but not this nonsense:

float q = ((struct S*)p)->f; // ub, cannot use lvalue of type `float` to access object of effective type `int`

... which just means that we can't make much use of TBAA when emitting struct copies in C.

In C++, on the other hand, the rule is even more garbage, since there is no way to perform a memory access with a glvalue of class type. (The closest you get is that a defaulted union construction/assignment copies the object representation, but that's expressed in terms of copying a sequence of unsigned chars, and in any case those are member functions and so already require an object of the correct type to exist.) See wg21.link/cwg2051

Apr 21 2017, 4:03 AM

Apr 20 2017

hfinkel accepted D32327: [OpenMP] Run libomptarget regression tests using all available system threads..

LGTM

Apr 20 2017, 6:41 PM · Restricted Project
hfinkel added a comment to D32199: [TySan] A Type Sanitizer (Clang).

If you're going to try to enforce the declared type of memory, you'll also need something like the C effective type rule to handle char buffers in C++. As far as I can tell, it's not actually legal under the spec to cast an array of chars to an arbitrary type and access it that way — you have to do something to establish that there's an object of that type there first.
If you memcpy'ed into that buffer from an object of the right type, that would be sufficient to create a new formal object of that type, but I don't see any way to sensibly apply that rule to e.g. the POSIX "read" function. It seems to me that you at least need to have a rule saying that it's okay to access a formal object of type char/char[] using an arbitrarily-typed l-value.

Apr 20 2017, 11:34 AM
hfinkel accepted D32301: Don't pass FPOpFusion::Strict to the backend.

LGTM

Apr 20 2017, 10:00 AM
hfinkel added a comment to D32199: [TySan] A Type Sanitizer (Clang).

As I've currently implemented it, both reads and writes set the type of previously-unknown storage, and after that it says fixed (unless you memcpy to it, memset it, or its lifetime ends (the type gets reset on lifetime.start/end and for malloc/allocas/etc.). There's a flag to enable the "writes always set the type" rule, but that's not the default. Is this too strict?

That seems like it will have at least three flavors of false positive:

  1. C's "effective type" rule allows writes to set the type pretty much unconditionally, unless the storage is for a variable with a declared type
Apr 20 2017, 8:39 AM
hfinkel added a comment to D32199: [TySan] A Type Sanitizer (Clang).

As I've currently implemented it, both reads and writes set the type of previously-unknown storage, and after that it says fixed (unless you memcpy to it, memset it, or its lifetime ends (the type gets reset on lifetime.start/end and for malloc/allocas/etc.). There's a flag to enable the "writes always set the type" rule, but that's not the default. Is this too strict?

That seems like it will have at least three flavors of false positive:

  1. C's "effective type" rule allows writes to set the type pretty much unconditionally, unless the storage is for a variable with a declared type
  2. After a placement new in C++, you should be able to use the storage as a new type
  3. Storing directly to a member access on a union (ie, with the syntax x.a = b) in C++ permits using the storage as the new type

    If we want to follow the relevant language rules by default, that would suggest that "writes always set the type" should be enabled by default in C and disabled by default in C++. That may not be the right decision for other reasons, though. In C++, writes through union members and new-expressions should probably (re)set the type
Apr 20 2017, 5:46 AM

Apr 19 2017

hfinkel abandoned D32260: [TBAA] Vector types should (only) alias with their element types.

@rjmccall said, on this topic, in D31885:

Apr 19 2017, 8:52 PM
hfinkel added a comment to D31561: cmath: Skip Libc for integral types in isinf, etc..

Marshall, that's what I assumed originally, but I figured Hal had some non-standard-but-worth-supporting use case in mind.

Apr 19 2017, 8:49 PM
hfinkel created D32260: [TBAA] Vector types should (only) alias with their element types.
Apr 19 2017, 5:14 PM
hfinkel added a comment to D32199: [TySan] A Type Sanitizer (Clang).

! In D32199#731252, @hfinkel wrote:

How about renaming this to something more like -fsanitize=type?

I'm fine with that. Do you like TypeSanitizer or TypeAccessSantizer or TypeAliasingSanitizer best?

I think calling it a type aliasing sanitizer would somewhat conflate the details of the mechanism with the fundamentals of the check itself. For example:

variant<int, float> v;
int &n = v.get<int>;
v = 1.3f;
int m = n;

... is a lifetime bug, not an aliasing bug, but would be caught by this check just the same.

Apr 19 2017, 5:04 PM
hfinkel added a comment to D31885: Remove TBAA information from LValues representing union members.

...

(And the nonsensicality of the standard very much continues. The code that we've all agreed is obviously being miscompiled here is still a strict violation of the "improved" union rules in C++, and if we decide to treat it as ill-formed we're going to break a massive amount of code (and vector code in particular heavily uses unions), because trust me, nobody is reliably placement-newing union fields when they're non-trivial. C++'s new rules are unworkable in part because they rely on C++-specific features that cannot be easily adopted in headers which need to be language-neutral.)

I think we all agree that the vector types need to have their scalar (element) types as parents in the current TBAA representation. That's a separate change that we should just make. That, unfortunately, only fixes a subset of the union-related miscompiles.

Apr 19 2017, 4:21 PM
hfinkel added a comment to D32199: [TySan] A Type Sanitizer (Clang).

...

I would also expect that we will extend this in future to assign types to storage even in cases where there is no store (for instance, we should be able to catch float f() { int n; return *(float*)&n; } despite there being no TBAA violation in the naive IR).

Yes. My thought was that we'd make Clang generate tbaa metadata on allocas and lifetime.start intrinsics (and globals) so that we can mark the memory types upon creation. Would that catch everything?

Apr 19 2017, 3:59 PM
hfinkel added a comment to D32199: [TySan] A Type Sanitizer (Clang).

...

I would also expect that we will extend this in future to assign types to storage even in cases where there is no store (for instance, we should be able to catch float f() { int n; return *(float*)&n; } despite there being no TBAA violation in the naive IR).

Apr 19 2017, 3:54 PM
hfinkel updated the diff for D32197: [TySan] A Type Sanitizer (Runtime Library).

Updated per review comments.

Apr 19 2017, 3:43 PM
hfinkel added a comment to D32197: [TySan] A Type Sanitizer (Runtime Library).

I just did a very quick first pass. Looks good in general, but I have some nits.

Apr 19 2017, 3:39 PM
hfinkel updated the diff for D32199: [TySan] A Type Sanitizer (Clang).

Updated per review comments (use only no_sanitize("tbaa") instead of adding no_sanitize_tbaa and don't touch freebsd for now).

Apr 19 2017, 2:59 PM
hfinkel added a comment to D32199: [TySan] A Type Sanitizer (Clang).

Missing a sanitizer-ld.c test for freebsd.

Apr 19 2017, 2:55 PM
hfinkel added a comment to D31885: Remove TBAA information from LValues representing union members.

I'm somewhat concerned that this patch is quadratic in the AST.

I'd be happy to address this, but I'm not sure how. Memoizing results could be one way, but don't know if that's acceptable.

This location in codegen seems to be the last place where the original C/C++ types are available, in particular the information as to whether a type is a union or not. Maybe it would be possible to propagate some bit somewhere, but then this patch would become much less localized.

Apr 19 2017, 11:57 AM
hfinkel added a comment to D31885: Remove TBAA information from LValues representing union members.

There was a deliberate decision to make TBAA conservative about type punning in LLVM because, in practice, the strict form of TBAA you think we should follow has proven to be a failed optimization paradigm: it just leads users to globally disable TBAA, which is obviously a huge loss. This was never motivated purely by the nonsensicality of the standardization of unions.

Apr 19 2017, 11:54 AM
hfinkel added a comment to D32198: [TySan] A Type Sanitizer (LLVM).

As a note: As follow-up work, we might want to extend Clang to add TBAA metadata to allocas and lifetime.start intrinsics so that the instrumentation pass can mark the types of memory upon declaration/construction instead of only upon first access.

Apr 19 2017, 5:59 AM
hfinkel added a comment to D32199: [TySan] A Type Sanitizer (Clang).

As a note: As follow-up work, we might want to extend Clang to add TBAA metadata to allocas and lifetime.start intrinsics so that the instrumentation pass can mark the types of memory upon declaration/construction instead of only upon first access.

Apr 19 2017, 5:58 AM

Apr 18 2017

hfinkel added a comment to D31885: Remove TBAA information from LValues representing union members.

To be clear, I'm find with disabling union tbaa until we can fix things for real. I'm somewhat concerned that this patch is quadratic in the AST.

Apr 18 2017, 8:11 PM
hfinkel added a reviewer for D31885: Remove TBAA information from LValues representing union members: rjmccall.
Apr 18 2017, 8:04 PM
hfinkel added a comment to D32198: [TySan] A Type Sanitizer (LLVM).

https://reviews.llvm.org/D32197 (Runtime)
https://reviews.llvm.org/D32199 (Clang)

Apr 18 2017, 4:17 PM
hfinkel added a comment to D32197: [TySan] A Type Sanitizer (Runtime Library).

https://reviews.llvm.org/D32198 (LLVM)
https://reviews.llvm.org/D32199 (Clang)

Apr 18 2017, 4:16 PM
hfinkel created D32199: [TySan] A Type Sanitizer (Clang).
Apr 18 2017, 4:15 PM
hfinkel created D32198: [TySan] A Type Sanitizer (LLVM).
Apr 18 2017, 4:04 PM
hfinkel updated the diff for D32197: [TySan] A Type Sanitizer (Runtime Library).

Upload the version of the patch that works on ppc64le.

Apr 18 2017, 4:00 PM
hfinkel created D32197: [TySan] A Type Sanitizer (Runtime Library).
Apr 18 2017, 3:58 PM

Apr 14 2017

hfinkel added a comment to D31885: Remove TBAA information from LValues representing union members.

I'm not sure this is the right way to do this; I suspect we're lumping together a bunch of different bugs:

  1. vector types need to have tbaa which makes them alias with their element types [to be clear, as vector types are an implementation extension, this is our choice; I believe most users expect this to be true, but I'm certainly open to leaving this as-is (i.e. the vector types and scalar types as independent/non-aliasing)].
  2. tbaa can't be used for write <-> write queries (only read <-> write queries) because the writes can change the effective type
  3. our 'struct' path TBAA for unions is broken (and to fix this we need to invert the tree structure, etc. as discussed on the list)

See https://bugs.llvm.org/show_bug.cgi?id=28189 for a testcase for (2) for this which doesn't involve unions.

Apr 14 2017, 11:56 AM
hfinkel added a comment to D31885: Remove TBAA information from LValues representing union members.

I'm not sure about the solution to #2, because i thought there were very specific points in time at which the effective type could change.

Apr 14 2017, 10:09 AM
hfinkel added a comment to D31885: Remove TBAA information from LValues representing union members.

This is not meant as a fine-grained solution to the TBAA problem, but a temporary fix for generating wrong information. Just yesterday I helped diagnose yet another problem related to this, so this issue is causing trouble out there.

Apr 14 2017, 9:12 AM
hfinkel added inline comments to D31167: Use FPContractModeKind universally.
Apr 14 2017, 5:54 AM
hfinkel added a comment to D31885: Remove TBAA information from LValues representing union members.

I'm not sure this is the right way to do this; I suspect we're lumping together a bunch of different bugs:

  1. vector types need to have tbaa which makes them alias with their element types [to be clear, as vector types are an implementation extension, this is our choice; I believe most users expect this to be true, but I'm certainly open to leaving this as-is (i.e. the vector types and scalar types as independent/non-aliasing)].
  2. tbaa can't be used for write <-> write queries (only read <-> write queries) because the writes can change the effective type
  3. our 'struct' path TBAA for unions is broken (and to fix this we need to invert the tree structure, etc. as discussed on the list)
Apr 14 2017, 5:45 AM
hfinkel added a reviewer for D31885: Remove TBAA information from LValues representing union members: dberlin.
Apr 14 2017, 5:38 AM

Apr 13 2017

hfinkel created D32033: Make test/parallel/omp_nested.c not use so many threads.
Apr 13 2017, 10:37 AM

Apr 12 2017

hfinkel accepted D31998: [msan] Fix msan_test broken after r299884..

LGTM, let's try it. It would be good to commit the vector fix separately in case we need to revert the interface change we shouldn't lose the bug fix too.

Apr 12 2017, 5:36 PM
hfinkel accepted D31531: Remove readnone from invariant.group.barrier.
Apr 12 2017, 1:37 PM
hfinkel added inline comments to D31167: Use FPContractModeKind universally.
Apr 12 2017, 11:06 AM
hfinkel accepted D30941: Better testing of schedule model instruction latencies/throughputs.

I implemeted all requirements from hfinkel.
Please, review again.

Apr 12 2017, 8:42 AM

Apr 11 2017

hfinkel added inline comments to D30941: Better testing of schedule model instruction latencies/throughputs.
Apr 11 2017, 11:25 AM
hfinkel added inline comments to D31167: Use FPContractModeKind universally.
Apr 11 2017, 10:12 AM
hfinkel added inline comments to D31167: Use FPContractModeKind universally.
Apr 11 2017, 9:45 AM
hfinkel added a comment to D31413: [libc++] Use __attribute__((init_priority(101))) to ensure streams get initialized early.

I'm not certain of a good way to test it, but I have a question about the value you picked for init_priority. My understanding of the values starting from 101 is that 1-100 are reserved for implementation use. Is that understanding correct? If so, you may want to pick a value below 100 to ensure there's not an arms race with the user. I believe this may require some alteration to SemaDeclAttr.cpp to not diagnose when this is coming from a system header. Otherwise, what's to stop the user from having something marked constructor(101) that attempts to use a stream, but can't because they're not initialized yet?

Apr 11 2017, 4:21 AM

Apr 10 2017

hfinkel added a comment to D31398: [X86][X86 intrinsics]Folding cmp(sub(a,b),0) into cmp(a,b) optimization.

(x-y) == 0 --> x == y does not require nsz (zeros of any sign compare equal), nor does it require nnan (if x or y is NaN, both comparisons are false). It *does* require ninf (because inf-inf = NaN), and it also requires that subnormals are not flushed by the CPU. There's been some churn around flushing recently, Hal may have thoughts (+Hal).

Apr 10 2017, 8:05 PM
hfinkel committed rL299911: [LICM] Hoist fp division from the loops and replace by a reciprocal.
[LICM] Hoist fp division from the loops and replace by a reciprocal
Apr 10 2017, 7:35 PM
hfinkel closed D30819: Fix PR32157 - Hoist fp division from the loops and replace by a reciprocal by committing rL299911: [LICM] Hoist fp division from the loops and replace by a reciprocal.
Apr 10 2017, 7:35 PM
hfinkel committed rL299910: [PowerPC] multiply-with-overflow might use the CTR register.
[PowerPC] multiply-with-overflow might use the CTR register
Apr 10 2017, 7:16 PM
hfinkel closed D31790: [PowerPC] Assume 128bit multiply uses CTR by committing rL299910: [PowerPC] multiply-with-overflow might use the CTR register.
Apr 10 2017, 7:15 PM
hfinkel committed rL299908: [bugpoint] Also remove comdat's from externalized GVs.
[bugpoint] Also remove comdat's from externalized GVs
Apr 10 2017, 5:31 PM
hfinkel accepted D31555: [PPC64, Sanitizers] Proper stack frame for the thread spawned in internal_clone.

LGTM

Apr 10 2017, 4:03 PM
hfinkel updated subscribers of D31561: cmath: Skip Libc for integral types in isinf, etc..

I'm happy with this now. @EricWF , @mclow.lists , thoughts?

Apr 10 2017, 3:49 PM
hfinkel added inline comments to D31167: Use FPContractModeKind universally.
Apr 10 2017, 1:03 PM
hfinkel added inline comments to D31167: Use FPContractModeKind universally.
Apr 10 2017, 12:38 PM
hfinkel added inline comments to D31561: cmath: Skip Libc for integral types in isinf, etc..
Apr 10 2017, 12:28 PM
hfinkel accepted D31790: [PowerPC] Assume 128bit multiply uses CTR.

@hfinkel: Thanks for the feedback!

I have made the changes recommended and uploaded a new revision. (I though this would trigger another mail but apparently it didn't, so I'm commenting again).

Apr 10 2017, 11:49 AM

Apr 9 2017

hfinkel committed rL299826: [PowerPC] Change -faltivec to -maltivec.
[PowerPC] Change -faltivec to -maltivec
Apr 9 2017, 10:28 AM
hfinkel committed rL299823: [MemorySSA] Fix use of pointsToConstantMemory in….
[MemorySSA] Fix use of pointsToConstantMemory in…
Apr 9 2017, 6:11 AM