- User Since
- Jan 18 2013, 11:30 AM (366 w, 4 d)
Please test the cases where the result is used; this should be handled for you automatically, but still.
Please test prefix increment/decrement as well.
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.
Fri, Jan 24
Thu, Jan 23
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.
Wed, Jan 22
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?
LGTM other than the request to split the commit.
Tue, Jan 21
Thanks, this looks right, assuming you've decided to continue calling them "objc blocks" despite my comment.
Mon, Jan 20
Fri, Jan 17
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.
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.
Thu, Jan 16
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.
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.
Tue, Jan 14
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?
Mon, Jan 13
Okay. This seems conceptually reasonable to me, but I don't feel qualified to review the code, sorry.
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.
Fri, Jan 10
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?
Thu, Jan 9
Wed, Jan 8
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.
Tue, Jan 7
I mean, I made a recommendation and you dismissed it.
It's not unusual for new warnings to require changes to other tests. I agree with enabling this by default.
If the committee *isn't* taking this up, they absolutely should, though.
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.
Mon, Jan 6
Fri, Jan 3
Thu, Jan 2
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.
Mon, Dec 30
Dec 22 2019
Dec 20 2019
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 19 2019
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 18 2019
We do have a static assert. I won't insist on the comment.
Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM.
Dec 17 2019
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.
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?
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 16 2019
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()).
Maybe this should depend on linkage?