Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (366 w, 4 d)

Recent Activity

Today

rjmccall accepted D73182: [CodeGen] Emit IR for fixed-point multiplication and division..
Tue, Jan 28, 10:21 AM · Restricted Project
rjmccall added a comment to D73184: [CodeGen] Emit IR for compound assignment with fixed-point operands..

Please test the cases where the result is used; this should be handled for you automatically, but still.

Tue, Jan 28, 10:21 AM · Restricted Project
rjmccall added a comment to D73183: [CodeGen] Emit IR for fixed-point unary operators..

Please test prefix increment/decrement as well.

Tue, Jan 28, 10:11 AM · Restricted Project
rjmccall requested changes to D73360: [OpenCL] Restrict address space conversions in nested pointers.
Tue, Jan 28, 10:02 AM
rjmccall accepted D73545: Fix a crash when casting _Complex and ignoring the results.

LGTM

Tue, Jan 28, 10:02 AM

Yesterday

rjmccall updated subscribers of D72582: [X86] Don't inherit 32-bit calling conventions in 64-bit mode..

I don't think we can just break ABI for a type like <3 x double> that can currently be produced by frontends like Clang and Swift; being sensitive to -mavx doesn't mean we don't care about the ABI at all. CC'ing @scanon in case he has other thoughts.

Mon, Jan 27, 1:48 PM · Restricted Project

Fri, Jan 24

rjmccall accepted D73187: [AST] Add fixed-point division constant evaluation..

LGTM

Fri, Jan 24, 8:50 AM · Restricted Project

Thu, Jan 23

rjmccall added inline comments to D73186: [AST] Add fixed-point multiplication constant evaluation..
Thu, Jan 23, 6:16 PM · Restricted Project
rjmccall accepted D73185: [AST] Add fixed-point subtraction constant evaluation..

LGTM.

Thu, Jan 23, 9:11 AM · Restricted Project
rjmccall accepted D73257: [AST] Compress the FixedPointSemantics type better..

Thanks, LGTM.

Thu, Jan 23, 9:11 AM · Restricted Project
rjmccall added inline comments to D73186: [AST] Add fixed-point multiplication constant evaluation..
Thu, Jan 23, 9:11 AM · Restricted Project
rjmccall added a comment to D72959: Relative VTables ABI on Fuchsia.

Okay. In that case, I would ask that if you're considering going through the work of adding relocations, please consider adding both scaled and unscaled relative-offset-to-equivalent-symbol.

Thu, Jan 23, 8:52 AM · Restricted Project, Restricted Project

Wed, Jan 22

rjmccall added a comment to D72959: Relative VTables ABI on Fuchsia.

There's two independently-useful things here for the relocation, right? First, it's useful to be able to express a relocation to a symbol that has the semantics of a particular function but doesn't necessarily have its address, and that concept could be used across many "physical" relocations; and second, it's potentially useful to have a shifted-by-two relative address relocation, at least on AArch64 where instructions (and v-table entries under this ABI) are always four-byte-aligned anyway. Is it possible to separate these concerns in ELF, e.g. by having a "symbol" that can be the target of any other relocation but which actually represents a function of unspecified address with the semantics of another function?

Wed, Jan 22, 7:30 PM · Restricted Project, Restricted Project
rjmccall accepted D73185: [AST] Add fixed-point subtraction constant evaluation..

LGTM other than the request to split the commit.

Wed, Jan 22, 7:12 PM · Restricted Project
rjmccall added inline comments to D73186: [AST] Add fixed-point multiplication constant evaluation..
Wed, Jan 22, 7:12 PM · Restricted Project

Tue, Jan 21

rjmccall accepted D72708: [libc++] Make sure std::is_object returns true for block types.

Thanks, this looks right, assuming you've decided to continue calling them "objc blocks" despite my comment.

Tue, Jan 21, 1:28 PM · Restricted Project

Mon, Jan 20

rjmccall added a comment to D72543: [libcxxabi] Insert padding in __cxa_exception struct for compatibility.

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

If __cxa_exception is already sufficiently aligned, I believe that patch does not allocate any extra space.

You're right; I understand the case rL319123 was intended for now.

Sorry I was on vacation. I intended to keep rL319123 as least for now. The current setup for MacOS is that clang header is setup to use unwind.h from SDK, which it will currently find the unaligned exception struct. After this patch, rL319123 is a no-op for people building libcxxabi against TOT llvm libunwind but it will help people building against the old header to get the correct alignment fro exception struct.

@rjmccall are you suggesting to put the static_assert directly in the header so users can get build failures if they expect different layout? That can help but I need to figure out what is the correct layout different users are expecting.

Mon, Jan 20, 6:16 PM · Restricted Project

Fri, Jan 17

rjmccall accepted D72970: clang: Only define OBJC_NEW_PROPERTIES when -x objective-c.

LGTM

Fri, Jan 17, 6:01 PM
rjmccall added a comment to D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP..

Remember that the design is that constrained intrinsics must be used whenever *any* code in the function is constrained. It is not unreasonable that part of the function might be constrained and the rest subject to fast-math; it'd be a shame if the intrinsics couldn't even express that.

Fri, Jan 17, 3:30 PM · Restricted Project, Restricted Project
rjmccall accepted D72897: List implicit operator== after implicit destructors in a vtable..

It wouldn't be a bad idea to track that correspondence in the AST just on the general principle that it's useful information (for diagnostics, tooling, etc.) that's otherwise hard to recreate. But I agree that it's also not problematic to expect Sema to add them in the proper order, and if we don't have an ABI that needs the correspondence directly (e.g. because it wants to emit the operators in pairs), it's fine to rely on it.

Fri, Jan 17, 3:11 PM · Restricted Project

Thu, Jan 16

rjmccall added a comment to D72897: List implicit operator== after implicit destructors in a vtable..

I see two reasonable approaches here: we can rely on the implicit members being added in the right order by Sema, as this patch seems to do, or we can broaden the special treatment of implicit virtual functions in the v-table layout code. The latter is more complex (we'd basically need to collect and sort all the implicit virtual functions), but it reliably isolates the ABI from implementation decisions like the order in which Sema processes implicit members and whether some lookup might have triggered an implicit declaration at a point we didn't expect.

Thu, Jan 16, 9:39 PM · Restricted Project
rjmccall added a comment to D72543: [libcxxabi] Insert padding in __cxa_exception struct for compatibility.

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

Thu, Jan 16, 4:02 PM · Restricted Project
rjmccall added a comment to D72543: [libcxxabi] Insert padding in __cxa_exception struct for compatibility.

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

Thu, Jan 16, 2:34 PM · Restricted Project
rjmccall added inline comments to D72114: [MS] Overhaul how clang passes overaligned args on x86_32.
Thu, Jan 16, 10:22 AM · Restricted Project
rjmccall added a comment to D72742: Don't assume promotable integers are zero/sign-extended already in x86-64 ABI..

Didn't we work this out already when John added the alignment tracking stuff? I remember this bug involving libjpegturbo standalone assembly receiving a 32-bit argument, and then using the full 64-bit RDI register to read it, but clang stopped zero extending it.

Thu, Jan 16, 10:13 AM · Restricted Project, Restricted Project

Tue, Jan 14

rjmccall added a comment to D71467: [FPEnv] Generate constrained FP comparisons from clang.

Well, just like for all the other FP builder methods, you can use the setIsFPConstrained method on the builder object to switch between strict and non-strict mode. Does this not suffice, or is there anything particular about the comparisons that would require anything extra?

Tue, Jan 14, 1:38 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D72708: [libc++] Make sure std::is_object returns true for block types.
Tue, Jan 14, 10:04 AM · Restricted Project
rjmccall added inline comments to D72708: [libc++] Make sure std::is_object returns true for block types.
Tue, Jan 14, 9:55 AM · Restricted Project

Mon, Jan 13

rjmccall resigned from D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Okay. This seems conceptually reasonable to me, but I don't feel qualified to review the code, sorry.

Mon, Jan 13, 7:45 PM · Restricted Project
rjmccall added a comment to D71467: [FPEnv] Generate constrained FP comparisons from clang.

Is this approach going to work with scope-local strictness? We need a way to do a comparison that has the non-strict properties but appears in a function that enables strictness elsewhere.

Mon, Jan 13, 1:24 PM · Restricted Project, Restricted Project
rjmccall added a comment to D72543: [libcxxabi] Insert padding in __cxa_exception struct for compatibility.

Do we need to worry about compatibility? What happens if there is a mix between an old binary and a new binary?

Mon, Jan 13, 12:46 PM · Restricted Project

Fri, Jan 10

rjmccall added a comment to D72543: [libcxxabi] Insert padding in __cxa_exception struct for compatibility.

Do we need to be careful about the alignment used by __attribute__((aligned)) on different targets? And would be it useful to put the static_asserts in the headers to make it more likely that we catch problems?

Fri, Jan 10, 4:34 PM · Restricted Project
rjmccall added inline comments to D72411: [CodeGen] partially revert 2b4fa5348ee157b6b1a1af44d0137ca8c7a71573 to fix Objective-C static variable initialization.
Fri, Jan 10, 3:11 PM · Restricted Project

Thu, Jan 9

rjmccall committed rG751c0ed2bc85: Apparently they corrected the spelling in trunk. (authored by rjmccall).
Apparently they corrected the spelling in trunk.
Thu, Jan 9, 5:07 PM
rjmccall committed rGd87a4a047dc0: Merge remote-tracking branch 'llvm.org/master' into apple/master (authored by rjmccall).
Merge remote-tracking branch 'llvm.org/master' into apple/master
Thu, Jan 9, 5:07 PM
rjmccall committed rGa94321f13043: Abstract serialization fixes for the Apple Clang changes. (authored by rjmccall).
Abstract serialization fixes for the Apple Clang changes.
Thu, Jan 9, 5:07 PM
GitHub <noreply@github.com> committed rG21e1fc51f302: Merge pull request #279 from smeenai/apple/master (authored by rjmccall).
Merge pull request #279 from smeenai/apple/master
Thu, Jan 9, 3:41 PM
rjmccall added a comment to D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese.

Most uses of the destructor do not use the delete operator, though, and therefore should not trigger the diagnostics in f to be emitted. And this really doesn't require a fully-realized use graph; you could very easily track the current use stack when making a later pass over the entities used.

The call graph is not for this specific situation. A call graph is needed because of the transitive nature of the deferred diagnostic message. That is, if any direct or indirect caller is emitted, the diagnostic msg needs to be emitted.

Thu, Jan 9, 11:57 AM
rjmccall added a comment to D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese.

I thought you were saying that the destructor decl hadn't been created yet, but I see now that you're saying something more subtle.

CurContext is set to the destructor because the standard says in [class.dtor]p13:

At the point of definition of a virtual destructor (including an implicit definition), the non-array deallocation function is determined as if for the expression `delete this` appearing in a non-virtual destructor of the destructor’s class.

Which is to say that, semantically, the context is as if it were within the destructor, to the extent that this affects access control and so on.

I can see why this causes problems for your call graph (really a use graph), since it's a use in the apparent context of the destructor at a point where the destructor is not being defined. A similar thing happens with default arguments, but because we don't consider uses from default arguments to be true ODR-uses until the default argument is used, that probably doesn't cause problems for you.

I don't think the destructor -> deallocation function edge is actually interesting for your use graph. It'd be more appropriate to treat the deallocation function as used by the v-table than by the destructor; I don't know whether you make any attempt to model v-tables as nodes in your use graph. You might consider finding a simple way to suppress adding this edge, like just not adding edges from a destructor that's not currently being defined (D->willHaveBody()).

With all that said, maintaining a use graph for all the functions you might emit in the entire translation unit seems very expensive and brittle. Have you considered doing this walk in a final pass? You could just build up a set of all the functions you know you're going to emit and then walk their bodies looking for uses of lazy-emitted entities. If we don't already have a function that calls a callback for every declaration ODR-used by a function body, we should.

The deferred diagnostic mechanism is shared between CUDA/HIP and OpenMP. The diagnostic messages not only depend on the callee, but also depend on the caller, the caller information needs to be kept. Also if a caller is to be emitted, all the deferred diagnostics associated with the direct or indirect callees need to be emitted. Therefore a call graph is needed for this mechanism.

If we ignore the dtor->deallocation edge in the call graph, we may miss diagnostics, e.g.

static __device__ __host__ void f(__m256i *p) {
  __asm__ volatile("vmovaps  %0, %%ymm0" ::"m"(*(__m256i *)p)
                 : "r0"); // MS-error{{unknown register name 'r0' in asm}}
}
struct CFileStream {
  void operator delete(void *p) {
    f(0);  // MS-note{{called by 'operator delete'}}
  }
  CFileStream();
  virtual ~CFileStream();  // MS-note{{called by '~CFileStream'}}
};
 
struct CMultiFileStream {
  CFileStream m_fileStream;
  ~CMultiFileStream();
};
 
// This causes vtable emitted so that deleting dtor is emitted for MS.
CFileStream::CFileStream() {}

Assuming the host compilation is on windows.

Here f() is a host device function which is unknown to be emitted, therefore the inline assembly error results in a delayed diagnostic. When f() is checked in the delete operator body, a 'delete operator -> f' edge is added to the call graph since f() is unknown to be emitted.

Since CFileStream::CFileStream is defined, clang sets vtbl to be emitted and does an explicit dtor check even though dtor is not defined. clang knows that this dtor check is for deleting dtor and will check delete operator as referenced, which causes `dtor -> delete operator' to be added to the call graph. Then clang checks dtor as referenced. Since deleting dtor will be emitted together with vtbl, clang should assume dtor is to be emitted. Then clang will found the callees 'delete operator' and f(), and emits the delayed diagnostics associated with them.

If we do not add 'dtor -> delete operator' edge to the call graph, the diagnostic msg in f() will not be emitted.

Thu, Jan 9, 10:43 AM
rjmccall accepted D72271: [Clang] Handle target-specific builtins returning aggregates..

Thanks, LGTM!

Thu, Jan 9, 8:59 AM · Restricted Project

Wed, Jan 8

rjmccall added inline comments to D72411: [CodeGen] partially revert 2b4fa5348ee157b6b1a1af44d0137ca8c7a71573 to fix Objective-C static variable initialization.
Wed, Jan 8, 1:37 PM · Restricted Project
rjmccall added inline comments to D72271: [Clang] Handle target-specific builtins returning aggregates..
Wed, Jan 8, 10:21 AM · Restricted Project
rjmccall added a comment to D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese.

I thought you were saying that the destructor decl hadn't been created yet, but I see now that you're saying something more subtle.

Wed, Jan 8, 12:05 AM

Tue, Jan 7

rjmccall added a comment to D68578: [HIP] Fix device stub name.

I mean, I made a recommendation and you dismissed it.

Tue, Jan 7, 10:52 PM
rjmccall added a reviewer for D72231: [Sema] Adds the pointer-to-int-cast diagnostic: rjmccall.

It's not unusual for new warnings to require changes to other tests. I agree with enabling this by default.

Tue, Jan 7, 6:19 PM · Restricted Project
rjmccall accepted D72375: Disallow an empty string literal in an asm label.

LGTM

Tue, Jan 7, 4:21 PM
rjmccall added a comment to D50119: P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`.

If the committee is actively looking into this problem and considering multiple alternatives, I don't see a particular need to accept your proposal into Clang in advance of the committee's decision. Arguably that would even be somewhat inappropriate, since it might be seen as an endorsement of your proposal over the alternatives, which I don't believe anyone from Clang has looked into. Letting the committee make a decision also addresses our disagreements about the language design and avoids introducing unnecessary divergence if the eventually-accepted design doesn't match your proposal.

Having a workable patch that you've taken advantage of in various projects should count as implementation experience for the committee's purposes.

If the committee *isn't* taking this up, they absolutely should, though.

I see the situation as exactly the opposite of what you just said. We cannot get implementation experience without an implementation. I would like Clang to be that implementation. But if Clang refuses to implement anything that is not Standard C++, then we have a chicken-and-egg problem: nobody can experiment with TR on real codebases until a compiler supports it. I would like Clang to be that compiler.

Tue, Jan 7, 2:54 PM · Restricted Project
rjmccall added a comment to D50119: P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`.

If the committee *isn't* taking this up, they absolutely should, though.

Tue, Jan 7, 12:49 PM · Restricted Project
rjmccall added a comment to D50119: P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`.

If the committee is actively looking into this problem and considering multiple alternatives, I don't see a particular need to accept your proposal into Clang in advance of the committee's decision. Arguably that would even be somewhat inappropriate, since it might be seen as an endorsement of your proposal over the alternatives, which I don't believe anyone from Clang has looked into. Letting the committee make a decision also addresses our disagreements about the language design and avoids introducing unnecessary divergence if the eventually-accepted design doesn't match your proposal.

Tue, Jan 7, 12:39 PM · Restricted Project
rjmccall added a comment to D72271: [Clang] Handle target-specific builtins returning aggregates..

The current return value of EmitTargetBuiltinExpr will be the llvm::Value * corresponding to the last of a sequence of store instructions that writes the constructed aggregate into the return value slot. There's no need to actually use that Value for anything in this case, but we can still test that it's non-null, to find out whether EmitTargetBuiltinExpr has successfully recognized a builtin and generated some code, or whether we have to fall through to the ErrorUnsupported.

Tue, Jan 7, 9:37 AM · Restricted Project
rjmccall added inline comments to D72114: [MS] Overhaul how clang passes overaligned args on x86_32.
Tue, Jan 7, 9:37 AM · Restricted Project
rjmccall added inline comments to D72114: [MS] Overhaul how clang passes overaligned args on x86_32.
Tue, Jan 7, 12:08 AM · Restricted Project

Mon, Jan 6

rjmccall added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

Below is the code comment from the new patch explaining the new approach, please take a look and see if you have any questions/comments:

// If two globals with differing sizes end up in the same mergeable
// section that section can be assigned an incorrect entry size. Normally,
// the assembler avoids this by putting incompatible globals into
// differently named sections. However, globals can be explicitly assigned
// to a section by specifying the section name. In this case, if unique
// section names are available (-unique-section-names in LLVM) then we
// bin compatible globals into different mergeable sections with the same name.

Looks good up to here.

// Otherwise, if incompatible globals have been explicitly assigned to section by a
// fine-grained/per-symbol mechanism (e.g. via  _attribute_((section(“myname”)))) then
// we issue an error and the user can then change the section assignment. If the

No bueno. The Linux kernel has code where there are 2D global arrays of different entity sizes explicitly placed in different sections (together). GCC does not error for this, (it doesn't mark the section merge-able), neither should we.

I laid out a series of three options before:

  • Emit different object-file sections for different mergeability settings.
  • Only mark an object-file section as mergeable if all the symbols in it would have the same mergeability settings.
  • Stop implicitly using mergeability for "ordinary" sections (i.e. sections other than the string section).

Of the above, I really think #2 is the only complete solution.

Mon, Jan 6, 9:56 AM · Restricted Project
rjmccall added inline comments to D72271: [Clang] Handle target-specific builtins returning aggregates..
Mon, Jan 6, 9:38 AM · Restricted Project

Fri, Jan 3

rjmccall added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

I looked into these today. I think we can do the first of these. I have put a WIP patch up here: https://reviews.llvm.org/D72194. Could you comment on the approach taken?

Fri, Jan 3, 8:39 PM · Restricted Project
rjmccall added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Fri, Jan 3, 8:39 PM · Restricted Project
rjmccall added inline comments to D72114: [MS] Overhaul how clang passes overaligned args on x86_32.
Fri, Jan 3, 5:53 PM · Restricted Project
rjmccall added inline comments to D72114: [MS] Overhaul how clang passes overaligned args on x86_32.
Fri, Jan 3, 12:21 PM · Restricted Project
rjmccall added inline comments to D72114: [MS] Overhaul how clang passes overaligned args on x86_32.
Fri, Jan 3, 11:40 AM · Restricted Project

Thu, Jan 2

rjmccall added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

The solution described in that comment is not acceptable. We are not going to tell users that they cannot assign symbols explicitly to sections because LLVM is unable to promise not to miscompile if they do. It is LLVM's responsibility to correctly compile valid code; enabling mergeability for a section containing unnamed_addr symbols is an optimization, and if it is not safe, it needs to be disabled until we can figure out a way to make it safe.

Thu, Jan 2, 5:57 PM · Restricted Project

Mon, Dec 30

rjmccall accepted D72012: [CodeGen] Use IRBuilder::CreateFNeg for __builtin_conj.
Mon, Dec 30, 12:51 PM · Restricted Project
rjmccall added inline comments to D72010: [CodeGen] Use CreateFNeg in buildFMulAdd.
Mon, Dec 30, 12:51 PM · Restricted Project
rjmccall accepted D72010: [CodeGen] Use CreateFNeg in buildFMulAdd.
Mon, Dec 30, 12:51 PM · Restricted Project

Dec 22 2019

rjmccall added a comment to D69272: Enable '#pragma STDC FENV_ACCESS' in frontend.

@hfinkel proposed to use outlining to extract a block with the #pragma to separate function. It could be a basis for a full implementation.

I don't think outlining is a reasonable approach. Outlining has a *lot* of other performance consequences, and we'd have to support arbitrary control flow (e.g. goto) in and out of the outlined function, which adds a lot of frontend complexity in pursuit of something that ought be handled at the optimizer level.

The restriction on using the #pragma on top-level only may be considered as request for 'manual outlining'. User have to extract the piece of code that uses the pragma, solving problems like data passing and control flow.

Dec 22 2019, 10:22 PM · Restricted Project
rjmccall accepted D71805: [clang] [ast] CXXRecordDecl::getVisibleConversionFunctions() could be const.

LGTM.

Dec 22 2019, 1:18 PM · Restricted Project

Dec 20 2019

rjmccall accepted D71708: [OPENMP50]Codegen for nontemporal clause..

Okay, LGTM.

Dec 20 2019, 1:37 PM · Restricted Project
rjmccall added a comment to D71708: [OPENMP50]Codegen for nontemporal clause..

Thanks. Could you add some tests that nontemporal directives aren't disturbed by either (1) nested nontemporal directives or (2) nested directives of some other kind?

Dec 20 2019, 12:23 PM · Restricted Project
rjmccall added inline comments to D71708: [OPENMP50]Codegen for nontemporal clause..
Dec 20 2019, 12:23 PM · Restricted Project
rjmccall added a comment to D69272: Enable '#pragma STDC FENV_ACCESS' in frontend.
In D69272#1792877, @kpn wrote:

My understanding of this patch is that it only allows the #pragma at the top of each function. It doesn't allow it in blocks inside the function. So if a function has a block inside it that uses strict FP the patch doesn't change the rest of the function to use constrained FP with the settings like you said. And my question was asking if there was a way forward with this patch to a full implementation.

@hfinkel proposed to use outlining to extract a block with the #pragma to separate function. It could be a basis for a full implementation.

Dec 20 2019, 12:01 PM · Restricted Project
rjmccall added inline comments to D71708: [OPENMP50]Codegen for nontemporal clause..
Dec 20 2019, 11:01 AM · Restricted Project

Dec 19 2019

rjmccall added a comment to D70258: [OpenMP][IR-Builder] Introduce the finalization stack.

Okay, if this is just handling OpenMP-specific control flow and doesn't need to interact with what a reasonable frontend would do with cleanups, I'm appeased.

Dec 19 2019, 10:23 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D71708: [OPENMP50]Codegen for nontemporal clause..
Dec 19 2019, 8:30 PM · Restricted Project

Dec 18 2019

rjmccall added a comment to D71650: [AST] Fix compilation with GCC < 8 for MinGW.

We do have a static assert. I won't insist on the comment.

Dec 18 2019, 12:05 PM · Restricted Project
rjmccall added a comment to D71650: [AST] Fix compilation with GCC < 8 for MinGW.

Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM.

The post-patch form doesn't look that odd to me (and we wouldn't want one comment for every one of the existing enums that already are outside of the structs where they are used for bitfield sizes), but do you think a comment is warranted here on this one?

Dec 18 2019, 11:06 AM · Restricted Project
rjmccall added a comment to D71650: [AST] Fix compilation with GCC < 8 for MinGW.

Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM.

Dec 18 2019, 9:22 AM · Restricted Project

Dec 17 2019

rjmccall added a comment to D70258: [OpenMP][IR-Builder] Introduce the finalization stack.

That's what I figured. The thing is that that necessarily interacts correctly with everything in Clang's IRGen in ways that making a second stack that Clang's IRGen doesn't directly know about doesn't.

Dec 17 2019, 4:28 PM · Restricted Project, Restricted Project
rjmccall added a comment to D70258: [OpenMP][IR-Builder] Introduce the finalization stack.

So it's never that OpenMP has things that need to be finalized before exiting (e.g. if something in frontend-emitted code throws an exception), it's just that OpenMP might need to generate its own control flow that breaks through arbitrary scopes in the frontend?

Dec 17 2019, 3:58 PM · Restricted Project, Restricted Project
rjmccall added a comment to D70258: [OpenMP][IR-Builder] Introduce the finalization stack.

Introducing an IRBuilder-level finalization stack sounds like it's going to be a huge mess if your goal is to plug this into other frontends.

While I get that you don't want to review this, I would really like to understand why you think this would become a mess.

Dec 17 2019, 12:12 PM · Restricted Project, Restricted Project
rjmccall accepted D71582: llvm-diff: Perform structural comparison on GlobalVariables, if possible.

Okay.

Dec 17 2019, 11:15 AM · Restricted Project
rjmccall resigned from D70258: [OpenMP][IR-Builder] Introduce the finalization stack.

I opposed the creation of this library on these terms in the first place, so I'm pretty sure I'm not on the hook to review it. Introducing an IRBuilder-level finalization stack sounds like it's going to be a huge mess if your goal is to plug this into other frontends.

Dec 17 2019, 11:07 AM · Restricted Project, Restricted Project

Dec 16 2019

rjmccall added a comment to D71582: llvm-diff: Perform structural comparison on GlobalVariables, if possible.

No, that's not true; something with External linkage would not trip any of those conditions normally. ("Externally initialized" in particular is basically unrelated to linkage.) I think you want a condition something like L->getName() == R->getName() || (L->hasLocalLinkage() && R->hasLocalLinkage()).

Dec 16 2019, 10:25 PM · Restricted Project
rjmccall added a comment to D71582: llvm-diff: Perform structural comparison on GlobalVariables, if possible.

Maybe this should depend on linkage?

Dec 16 2019, 7:41 PM · Restricted Project
rjmccall committed rG803403afc83f: Fix a bug in the property-based serialization of dependent template names. (authored by rjmccall).
Fix a bug in the property-based serialization of dependent template names.
Dec 16 2019, 1:12 PM
rjmccall committed rGb699fe8b951e: Forward {read,write}SomeEnumType to {read,write}Enum instead of directly to… (authored by rjmccall).
Forward {read,write}SomeEnumType to {read,write}Enum instead of directly to…
Dec 16 2019, 10:35 AM
rjmccall committed rGda74c4d2d822: Use property-based serialization for TemplateArgument. (authored by rjmccall).
Use property-based serialization for TemplateArgument.
Dec 16 2019, 10:35 AM
rjmccall committed rG2e2d142efe56: Add Optional::map. (authored by rjmccall).
Add Optional::map.
Dec 16 2019, 10:35 AM
rjmccall committed rGa9db0d9f17f3: Use property-based serialization for TemplateName. (authored by rjmccall).
Use property-based serialization for TemplateName.
Dec 16 2019, 10:35 AM
rjmccall committed rG256ec9964462: Add the ability to declare helper variables when reading properties from a… (authored by rjmccall).
Add the ability to declare helper variables when reading properties from a…
Dec 16 2019, 10:35 AM
rjmccall committed rG6887ccfcf283: Add the ability for properties to be conditional on other properties. (authored by rjmccall).
Add the ability for properties to be conditional on other properties.
Dec 16 2019, 10:35 AM
rjmccall committed rG867570a2384e: Use property-based serialization for DeclarationName. (authored by rjmccall).
Use property-based serialization for DeclarationName.
Dec 16 2019, 10:35 AM
rjmccall committed rG41d935f2c619: Replace tabs with spaces. (authored by rjmccall).
Replace tabs with spaces.
Dec 16 2019, 10:35 AM
rjmccall committed rGefd0dfbd700d: Add the ability to use property-based serialization for "cased" types. (authored by rjmccall).
Add the ability to use property-based serialization for "cased" types.
Dec 16 2019, 10:35 AM
rjmccall committed rG00bc76edddb5: Move Basic{Reader,Writer} emission into ASTPropsEmitter; NFC. (authored by rjmccall).
Move Basic{Reader,Writer} emission into ASTPropsEmitter; NFC.
Dec 16 2019, 10:35 AM
rjmccall committed rGc82e4ef6960b: Always -I clang/include when tblgen'ing in Clang. (authored by rjmccall).
Always -I clang/include when tblgen'ing in Clang.
Dec 16 2019, 10:35 AM

Dec 14 2019

rjmccall committed rG2ac702aaf091: Move ASTRecordWriter into its own header; NFC. (authored by rjmccall).
Move ASTRecordWriter into its own header; NFC.
Dec 14 2019, 12:31 AM
rjmccall committed rGc2f18315ff53: Move ASTRecordReader into its own header; NFC. (authored by rjmccall).
Move ASTRecordReader into its own header; NFC.
Dec 14 2019, 12:31 AM

Dec 13 2019

rjmccall committed rGd14a5693c07f: MSVC build fix: forget some unneeded and incorrect friends. (authored by rjmccall).
MSVC build fix: forget some unneeded and incorrect friends.
Dec 13 2019, 10:03 PM
rjmccall committed rGd505e57cc273: Abstract serialization: TableGen the (de)serialization code for Types. (authored by rjmccall).
Abstract serialization: TableGen the (de)serialization code for Types.
Dec 13 2019, 9:29 PM
rjmccall committed rG6404bd236240: Abstract serialization: TableGen "basic" reader/writer CRTP classes that… (authored by rjmccall).
Abstract serialization: TableGen "basic" reader/writer CRTP classes that…
Dec 13 2019, 9:29 PM
rjmccall committed rG3ce3d23facf1: Standardize the reader methods in ASTReader; NFC. (authored by rjmccall).
Standardize the reader methods in ASTReader; NFC.
Dec 13 2019, 9:28 PM