[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Modules/Inputs/System/usr/include/stdint.h:2
 typedef int my_awesome_nonstandard_integer_type;
+
+/* C99 7.18.1.1 Exact-width integer types.

Bigcheese wrote:
> iana wrote:
> > dexonsmith wrote:
> > > benlangmuir wrote:
> > > > iana wrote:
> > > > > iana wrote:
> > > > > > benlangmuir wrote:
> > > > > > > Why do we need all this code now (I assume this is copied from 
> > > > > > > the real header)?
> > > > > > It's to support the tests I added to Modules/compiler_builtins.m. 
> > > > > > stdatomic.h and inttypes.h rely on stdint.h contents from the C 
> > > > > > library, and on complex.h and inttypes.h and math.h being present. 
> > > > > > I could just test the modules with `-ffreestanding` I think, would 
> > > > > > that be better?
> > > > > (and yes I just copied it from the builtin header)
> > > > I'm not sure what the best approach is here, but this seems heavy for a 
> > > > test.  Do we expect to need to keep this up to date? If so maybe 
> > > > ffreestanding for the specific test cases this affects would be a 
> > > > better tradeoff.  Maybe someone else has a suggestion here
> > > This reflects *most* of the content in `stdint.h`.
> > > 
> > > One idea would be to copy the full header over during the test setup and 
> > > fix it up, either:
> > > - strip out the header/footer or
> > > - `sed -e "s/__STDC_HOSTED__/0/g"`
> > > so that it always provides content (even when not freestanding).
> > IMO it doesn't need to be that sophisticated, the types just have to exist 
> > for stdtomic.h to compile. I could chop all the comments and define 
> > everything to `int` and I think that'd be fine too.
> Looking at stdatomic.h I think this approach would be fine. It just wants the 
> types to exist.
SGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159064/new/

https://reviews.llvm.org/D159064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D74094#4645562 , @nickdesaulniers 
wrote:

> I don't yet fully comprehend yet what's going wrong, and probably need to 
> familiarize myself with the language rules around `auto`'s type deduction.

For reduction purposes, it might be useful to factor out the auto type 
deduction on the return. I think you can do that with the help of `-Xclang 
-ast-dump`. E.g., for this function:

  auto f() { return 1; };

the AST dump tells me the return type is `int`:

  |-FunctionDecl 0x1508e5698  col:6 f 'int ()'
  | `-CompoundStmt 0x1508e5928 
  |   `-ReturnStmt 0x1508e5918 
  | `-IntegerLiteral 0x1508e5780  'int' 1

You could then replace the `auto` with the type given in the dump.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Modules/Inputs/System/usr/include/stdint.h:2
 typedef int my_awesome_nonstandard_integer_type;
+
+/* C99 7.18.1.1 Exact-width integer types.

benlangmuir wrote:
> iana wrote:
> > iana wrote:
> > > benlangmuir wrote:
> > > > Why do we need all this code now (I assume this is copied from the real 
> > > > header)?
> > > It's to support the tests I added to Modules/compiler_builtins.m. 
> > > stdatomic.h and inttypes.h rely on stdint.h contents from the C library, 
> > > and on complex.h and inttypes.h and math.h being present. I could just 
> > > test the modules with `-ffreestanding` I think, would that be better?
> > (and yes I just copied it from the builtin header)
> I'm not sure what the best approach is here, but this seems heavy for a test. 
>  Do we expect to need to keep this up to date? If so maybe ffreestanding for 
> the specific test cases this affects would be a better tradeoff.  Maybe 
> someone else has a suggestion here
This reflects *most* of the content in `stdint.h`.

One idea would be to copy the full header over during the test setup and fix it 
up, either:
- strip out the header/footer or
- `sed -e "s/__STDC_HOSTED__/0/g"`
so that it always provides content (even when not freestanding).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159064/new/

https://reviews.llvm.org/D159064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/Headers/__stddef_null.h:14
+ * __need_NULL and rely on stddef.h to redefine NULL to the correct value 
again.
+ * Modules don't support redefining macros like that, but support that pattern
+ * in the non-modules case.

iana wrote:
> dexonsmith wrote:
> > iana wrote:
> > > I couldn't come up with anything clever to support this in modules. If I 
> > > get rid of the guard, `NULL` still doesn't get redefined in modules. If I 
> > > move the `undef` to stddef.h then `NULL` gets undefined, but not 
> > > redefined. So I think precompiled modules must inherently have `#pragma 
> > > once` semantics. If we really had to, we could move this back into 
> > > stddef.h and let it be textual, but it's distasteful that `NULL` gets 
> > > defined in every single module that includes stddef.h and has to be 
> > > merged later.
> > Modules are, in a way, stronger than `#pragma once`. Each module is a 
> > separate translation unit. If there's a module around `__stddef_null.h`, 
> > then it will only be compiled once, and any definitions imported from 
> > there. Other importing TUs get those definitions per the context the module 
> > was compiled in (they don't re-compile the code in their own context...).
> > 
> > It seems like a regression for `NULL` to degrade from `((void *)0)` to `0` 
> > since it loses type safety, which is rather rare and precious in C code. 
> > Maybe this header should remain textual when deployed to platforms that 
> > rely on this dance.
> Sure, but I was a little surprised that this doesn't work.
> ```
> @import _Builtin_stddef.null;
> undef NULL
> @import _Builtin_stddef.null;
> // NULL is undefined
> ```
> It makes perfect sense that the module doesn't get re-compiled on the second 
> import (and maybe not even the first), and that when it does get precompiled 
> it doesn't see anything that the importing code did with `NULL`. But I //am// 
> a bit surprised that re-importing the module doesn't re-expose everything 
> from it.
> 
> I guess if we really need to, we could `unifdef` the module map in case some 
> platforms need this to be textual, and then lose the guard.
Yeah, subsequent imports have no effect. I can see how it's confusing in that 
situation, but it's important that they are redundant.

> I guess if we really need to, we could unifdef the module map in case some 
> platforms need this to be textual, and then lose the guard.

This makes sense to me, have it be either textual or a submodule depending on 
configuration for the platform. Maybe someone involved in supporting clang 
modules on Linux has an opinion though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159064/new/

https://reviews.llvm.org/D159064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/Headers/__stddef_null.h:14
+ * __need_NULL and rely on stddef.h to redefine NULL to the correct value 
again.
+ * Modules don't support redefining macros like that, but support that pattern
+ * in the non-modules case.

iana wrote:
> I couldn't come up with anything clever to support this in modules. If I get 
> rid of the guard, `NULL` still doesn't get redefined in modules. If I move 
> the `undef` to stddef.h then `NULL` gets undefined, but not redefined. So I 
> think precompiled modules must inherently have `#pragma once` semantics. If 
> we really had to, we could move this back into stddef.h and let it be 
> textual, but it's distasteful that `NULL` gets defined in every single module 
> that includes stddef.h and has to be merged later.
Modules are, in a way, stronger than `#pragma once`. Each module is a separate 
translation unit. If there's a module around `__stddef_null.h`, then it will 
only be compiled once, and any definitions imported from there. Other importing 
TUs get those definitions per the context the module was compiled in (they 
don't re-compile the code in their own context...).

It seems like a regression for `NULL` to degrade from `((void *)0)` to `0` 
since it loses type safety, which is rather rare and precious in C code. Maybe 
this header should remain textual when deployed to platforms that rely on this 
dance.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159064/new/

https://reviews.llvm.org/D159064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Now that the behaviour change is understood, maybe it'd be useful to split the 
patch in two:

- First, this patch, plus a call to `ASTReader::getInputFile()` only for its 
side effects, to make this patch actually NFC.
- Second, committed a few days later, a patch that removes the call and adds a 
test confirming `-I` is no longer implied by `-fembed-all-input-files`.

That way, the future temporary reverts won't churn the file format. For 
example, downstreams that hit problems and need to temporarily revert can just 
revert the second patch.

(... assuming that @rsmith/others confirm that implying `-I` is undesirable... 
or, if it's desired, surely there's a more explicit way to implement it.)




Comment at: clang/lib/Serialization/ASTWriter.cpp:3013
 
-SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FileIDAndFile.first, 
0);
-assert(!Loc.isInvalid() && "start loc for valid FileID is invalid");
-AddSourceLocation(Loc, Record);
+AddFileID(FileIDAndFile.first, Record);
 

This is where I'm suggesting a side-effects-only call to 
`ASTReader::getInputFile()` could be added. (Or to something that transitively 
will call it.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137213/new/

https://reviews.llvm.org/D137213

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 

MaskRay wrote:
> dexonsmith wrote:
> > MaskRay wrote:
> > > hans wrote:
> > > > MaskRay wrote:
> > > > > hans wrote:
> > > > > > dexonsmith wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > dexonsmith wrote:
> > > > > > > > > Why would we want to use the old name here? An alias seems 
> > > > > > > > > strictly better to me. 
> > > > > > > > Making `overriding-t-option` an alias for `overriding-option` 
> > > > > > > > would make `-Wno-overriding-t-option` applies to future 
> > > > > > > > overriding option diagnostics, which is exactly what I want to 
> > > > > > > > avoid.
> > > > > > > > 
> > > > > > > I understand that you don't want `-t-` to apply to work on future 
> > > > > > > overriding option diagnostics, but I think the platform 
> > > > > > > divergence you're adding here is worse.
> > > > > > > 
> > > > > > > Having a few Darwin-specific options use `-Woverriding-t-option` 
> > > > > > > (and everything else use `-Woverriding-option`) as the canonical 
> > > > > > > spelling is hard to reason about for maintainers, and for users.
> > > > > > > 
> > > > > > > And might not users on other platforms have 
> > > > > > > `-Woverriding-t-option` hardcoded in  build settings? (So 
> > > > > > > @dblaikie's argument that we shouldn't arbitrarily make things 
> > > > > > > hard for users would apply to all options?)
> > > > > > > 
> > > > > > > IMO, if we're not comfortable removing `-Woverriding-t-option` 
> > > > > > > entirely, then it should live on as an alias (easy to reason 
> > > > > > > about), not as canonical-in-special-cases (hard to reason about).
> > > > > > > IMO, if we're not comfortable removing -Woverriding-t-option 
> > > > > > > entirely, then it should live on as an alias (easy to reason 
> > > > > > > about), not as canonical-in-special-cases (hard to reason about).
> > > > > > 
> > > > > > +1 if we can't drop the old spelling, an alias seems like the best 
> > > > > > option.
> > > > > Making `overriding-t-option` an alias for `overriding-option`, as I 
> > > > > mentioned, will make `-Wno-overriding-t-option` affect new 
> > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > 
> > > > > I know there are some `-Wno-overriding-t-option` uses. Honestly, they 
> > > > > are far fewer than other diagnostics we are introducing or changing 
> > > > > in Clang. I can understand the argument "make -Werror users easier 
> > > > > for this specific diagnostic" (but `-Werror` will complain about 
> > > > > other new diagnostics), but do we really want to in the Darwin case? 
> > > > > I think no. They can remove the version from the target triple like 
> > > > > https://github.com/facebook/ocamlrep/blame/abc14b8aafcc6746ec37bf7bf0de24bfc58d63a0/prelude/apple/apple_target_sdk_version.bzl#L50
> > > > > 　or make the version part consistent with `-m.*os-version-min`.
> > > > > 
> > > > > This change may force these users to re-think how they should fix  
> > > > > their build. I apology to these users, but I don't feel that adding 
> > > > > an alias is really necessary.
> > > > > Making overriding-t-option an alias for overriding-option, as I 
> > > > > mentioned, will make -Wno-overriding-t-option affect new 
> > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > 
> > > > Is keeping them separate actually important, though? 
> > > > -Wno-overriding-option has the same issue in that case, that using the 
> > > > flag will also affect any new overriding-options uses, and I don't 
> > > > think that's a problem.
> > > `-Wno-overriding-option` is properly named, so affecting new 
> > > overriding-options uses looks fine to me.
> > > `-Wno-overriding-t-option` is awkward, and making it affect new uses 
> > > makes me nervous.
> > > 
> > > The gist of my previous comment is whether the uses cases really justify 
> > > a tiny bit of tech bit in clang and I think the answer is no.
> > > 
> > > This change is not about changing a semantic warning that has mixed 
> > > opinions, e.g. `-Wbitwise-op-parentheses` (many consider it not 
> > > justified).
> > > The gist of my previous comment is whether the uses cases really justify 
> > > a tiny bit of tech bit in clang and I think the answer is no.
> > > 
> > 
> > I think we agree that we should add the minimal technical debt to clang.
> > 
> > This patch is harder-to-reason about, and thus bigger IMO, technical debt 
> > than adding an alias would be.
> Honestly when people asked whether we need back compatibility for `-Werror` 
> uses. I disagree with the idea after 

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 

MaskRay wrote:
> hans wrote:
> > MaskRay wrote:
> > > hans wrote:
> > > > dexonsmith wrote:
> > > > > MaskRay wrote:
> > > > > > dexonsmith wrote:
> > > > > > > Why would we want to use the old name here? An alias seems 
> > > > > > > strictly better to me. 
> > > > > > Making `overriding-t-option` an alias for `overriding-option` would 
> > > > > > make `-Wno-overriding-t-option` applies to future overriding option 
> > > > > > diagnostics, which is exactly what I want to avoid.
> > > > > > 
> > > > > I understand that you don't want `-t-` to apply to work on future 
> > > > > overriding option diagnostics, but I think the platform divergence 
> > > > > you're adding here is worse.
> > > > > 
> > > > > Having a few Darwin-specific options use `-Woverriding-t-option` (and 
> > > > > everything else use `-Woverriding-option`) as the canonical spelling 
> > > > > is hard to reason about for maintainers, and for users.
> > > > > 
> > > > > And might not users on other platforms have `-Woverriding-t-option` 
> > > > > hardcoded in  build settings? (So @dblaikie's argument that we 
> > > > > shouldn't arbitrarily make things hard for users would apply to all 
> > > > > options?)
> > > > > 
> > > > > IMO, if we're not comfortable removing `-Woverriding-t-option` 
> > > > > entirely, then it should live on as an alias (easy to reason about), 
> > > > > not as canonical-in-special-cases (hard to reason about).
> > > > > IMO, if we're not comfortable removing -Woverriding-t-option 
> > > > > entirely, then it should live on as an alias (easy to reason about), 
> > > > > not as canonical-in-special-cases (hard to reason about).
> > > > 
> > > > +1 if we can't drop the old spelling, an alias seems like the best 
> > > > option.
> > > Making `overriding-t-option` an alias for `overriding-option`, as I 
> > > mentioned, will make `-Wno-overriding-t-option` affect new 
> > > overriding-options uses. This is exactly what I want to avoid.
> > > 
> > > I know there are some `-Wno-overriding-t-option` uses. Honestly, they are 
> > > far fewer than other diagnostics we are introducing or changing in Clang. 
> > > I can understand the argument "make -Werror users easier for this 
> > > specific diagnostic" (but `-Werror` will complain about other new 
> > > diagnostics), but do we really want to in the Darwin case? I think no. 
> > > They can remove the version from the target triple like 
> > > https://github.com/facebook/ocamlrep/blame/abc14b8aafcc6746ec37bf7bf0de24bfc58d63a0/prelude/apple/apple_target_sdk_version.bzl#L50
> > > 　or make the version part consistent with `-m.*os-version-min`.
> > > 
> > > This change may force these users to re-think how they should fix  their 
> > > build. I apology to these users, but I don't feel that adding an alias is 
> > > really necessary.
> > > Making overriding-t-option an alias for overriding-option, as I 
> > > mentioned, will make -Wno-overriding-t-option affect new 
> > > overriding-options uses. This is exactly what I want to avoid.
> > 
> > Is keeping them separate actually important, though? -Wno-overriding-option 
> > has the same issue in that case, that using the flag will also affect any 
> > new overriding-options uses, and I don't think that's a problem.
> `-Wno-overriding-option` is properly named, so affecting new 
> overriding-options uses looks fine to me.
> `-Wno-overriding-t-option` is awkward, and making it affect new uses makes me 
> nervous.
> 
> The gist of my previous comment is whether the uses cases really justify a 
> tiny bit of tech bit in clang and I think the answer is no.
> 
> This change is not about changing a semantic warning that has mixed opinions, 
> e.g. `-Wbitwise-op-parentheses` (many consider it not justified).
> The gist of my previous comment is whether the uses cases really justify a 
> tiny bit of tech bit in clang and I think the answer is no.
> 

I think we agree that we should add the minimal technical debt to clang.

This patch is harder-to-reason about, and thus bigger IMO, technical debt than 
adding an alias would be.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158301/new/

https://reviews.llvm.org/D158301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 

MaskRay wrote:
> dexonsmith wrote:
> > Why would we want to use the old name here? An alias seems strictly better 
> > to me. 
> Making `overriding-t-option` an alias for `overriding-option` would make 
> `-Wno-overriding-t-option` applies to future overriding option diagnostics, 
> which is exactly what I want to avoid.
> 
I understand that you don't want `-t-` to apply to work on future overriding 
option diagnostics, but I think the platform divergence you're adding here is 
worse.

Having a few Darwin-specific options use `-Woverriding-t-option` (and 
everything else use `-Woverriding-option`) as the canonical spelling is hard to 
reason about for maintainers, and for users.

And might not users on other platforms have `-Woverriding-t-option` hardcoded 
in  build settings? (So @dblaikie's argument that we shouldn't arbitrarily make 
things hard for users would apply to all options?)

IMO, if we're not comfortable removing `-Woverriding-t-option` entirely, then 
it should live on as an alias (easy to reason about), not as 
canonical-in-special-cases (hard to reason about).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158301/new/

https://reviews.llvm.org/D158301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 

Why would we want to use the old name here? An alias seems strictly better to 
me. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158301/new/

https://reviews.llvm.org/D158301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D158137#4597565 , @MaskRay wrote:

> In D158137#4597491 , @dexonsmith 
> wrote:
>
>> In D158137#4597009 , @MaskRay 
>> wrote:
>>
>>> In D158137#4596948 , @dexonsmith 
>>> wrote:
>>>
 Can you explain the downside of leaving behind an alias?
>>>
>>> Two minor ones. (a) Existing `-Wno-overriding-t-option` will not notice 
>>> that they need to migrate and (b) Clang has accrued tiny tech debt.
>>> If we eventually remove `-Wno-overriding-t-option` for tidiness, we will 
>>> have to break `-Werror -Wno-overriding-t-option` users.
>>
>> I guess it's not clear to me we'd need to remove the alias. The usual policy 
>> (I think?) is that clang driver options don't disappear. It seems like a 
>> small piece of debt to maintain the extra alias in this case, and if it's 
>> kept, then users don't actually need to migrate. And then you can feel safe 
>> updating Darwin.cpp as well.
>
> `-W*` options are different from regular driver options in that 
> `-Wunknown-unknown-unknown` leads to a warning instead of an error, while a 
> regular unrecognized driver option leads to an error.
> We deprecate driver options and make use of them warnings, and newer Clang 
> generally emits more warnings. These would break `-Werror` users as well, but 
> we still do them anyway if reasonable.
>
> I understand that it is a small piece of debt, but my point is that we don't 
> need the debt.

Okay, fine by me if others are happy!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158137/new/

https://reviews.llvm.org/D158137

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D158137#4597009 , @MaskRay wrote:

> In D158137#4596948 , @dexonsmith 
> wrote:
>
>> Can you explain the downside of leaving behind an alias?
>
> Two minor ones. (a) Existing `-Wno-overriding-t-option` will not notice that 
> they need to migrate and (b) Clang has accrued tiny tech debt.
> If we eventually remove `-Wno-overriding-t-option` for tidiness, we will have 
> to break `-Werror -Wno-overriding-t-option` users.

I guess it's not clear to me we'd need to remove the alias. The usual policy (I 
think?) is that clang driver options don't disappear. It seems like a small 
piece of debt to maintain the extra alias in this case, and if it's kept, then 
users don't actually need to migrate. And then you can feel safe updating 
Darwin.cpp as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158137/new/

https://reviews.llvm.org/D158137

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Can you explain the downside of leaving behind an alias?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158137/new/

https://reviews.llvm.org/D158137

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This seems to drop `-Woverriding-t-option` entirely. Could that break builds if 
someone has (e.g.) `-Werror -Wno-overriding-t-option` in their build settings?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158137/new/

https://reviews.llvm.org/D158137

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158137: Change -ffp-model= related warn_drv_overriding_flag_option to warn_drv_overriding_option

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.

Perhaps as a follow-up, rename warn_drv_overriding_flag_option to have “t” in 
it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158137/new/

https://reviews.llvm.org/D158137

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Nice!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89001: [clang] Don't look into for C++ headers if they are found alongside the toolchain

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

SGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89001/new/

https://reviews.llvm.org/D89001

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157283: [clang] Match -isysroot behaviour with system compiler on Darwin

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith resigned from this revision.
dexonsmith added subscribers: arphaman, jroelofs.
dexonsmith added a comment.

This looks correct to me, but I'd rather have someone at Apple confirm. 
@ldionne (or @arphaman or @jroelofs), can you review and/or help to find the 
right person?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157283/new/

https://reviews.llvm.org/D157283

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157011: [Clang][Tooling] Accept preprocessed input files

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157011/new/

https://reviews.llvm.org/D157011

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89001: [clang] Don't look into for C++ headers if they are found alongside the toolchain

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I don't have access to rdar these days to look into the current state or to 
refresh my memory.

My memory is that the high-level goal was to move the libc++ headers from the 
toolchain to the SDK. For the transition, this was staged in such a way that 
they were in both locations for some period of time (internally, the transition 
may be ongoing, for all I know).

Apple clang needed to prefer the SDK, since that was the "new" place. I don't 
remember the reasoning for inverting this behaviour for LLVM clang.

- Maybe, because this way an LLVM toolchain could still override the C++ 
headers, like it could previously?
- Maybe, there was a plan to eventually switch the default in LLVM clang, but 
that got dropped/forgotten?
- Maybe, there was a plan to eventually switch the default in Apple clang, but 
that was dropped/forgotten, or the internal transition is ongoing? (This rings 
a bell. "Change LLVM clang to the desired end state, and we'll flip the 
direction temporarily in Apple clang for the duration of the transition...")

Anyway, I'm not sure the best way forward here. Perhaps clang should have a 
flag to decide which is preferred. Or perhaps LLVM clang should just change to 
match Apple clang for now.

@ldionne, thoughts?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89001/new/

https://reviews.llvm.org/D89001

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added subscribers: arphaman, akyrtzi, jroelofs.
dexonsmith added a comment.

In D74094#4554327 , @foad wrote:

> Hi @erik.pilkington, I see this got reverted:

I'm not sure if @erik.pilkington is still watching Phabricator, but in any case 
I think (like me) he no longer has rdar access. Since this was a Linux PPC bot, 
it's possible Erik didn't get far investigating the failure. Three years later, 
the sands may have shifted substantially... maybe the best option to is rebase 
and investigate the new failures (if any).

@akyrtzi @arphaman @jroelofs, is one of you available to check rdar://58552124, 
in case it has extra context from the PPC bot failure?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/Sema/SemaLookup.cpp:542
+N = Decls.size();
+  }
+

john.brawn wrote:
> rjmccall wrote:
> > john.brawn wrote:
> > > rjmccall wrote:
> > > > This is going to fire on every single ordinary lookup that finds 
> > > > multiple declarations, right?  I haven't fully internalized the issue 
> > > > you're solving here, but this is a very performance-sensitive path in 
> > > > the compiler; there's a reason this code is written to very carefully 
> > > > only do extra work when we've detected in the loop below that we're in 
> > > > a hidden-declarations situation.  Is there any way we can restore that 
> > > > basic structure?
> > > Test4 in the added tests is an example of why we can't wait until after 
> > > the main loop. The `using A::X` in namespace D is eliminated by the 
> > > UniqueResult check, so the check for a declaration being hidden can only 
> > > see the using declarations in namespace C. We also can't do it in the 
> > > loop itself, as then we can't handle Test5: at the time we process the 
> > > `using A::X` in namespace D it looks like it may cause ambiguity, but 
> > > it's later hidden by the `using B::X` in the same namespace which we 
> > > haven't yet processed.
> > > 
> > > I have adjusted it though so the nested loop and erasing of decls only 
> > > happens when we both have things that hide and things that can be hidden. 
> > > Doing some quick testing of compiling SemaOpenMP.cpp (the largest file in 
> > > clang), LookupResult::resolveKind is called 75318 times, of which 75283 
> > > calls have HideTags=true, of which 56 meet this condition, i.e. 0.07%.
> > Okay, I can see why you need to not mix tag-hiding with the removal of 
> > duplicates.  However, I think you can maintain the current structure by 
> > delaying the actual removal of declarations until after the main loop; have 
> > the loop build up a set of indices to remove instead.  (Also, you can keep 
> > this set as a bitset instead of a `std::set`.)
> > 
> > It's a shame that the hiding algorithm has to check every other declaration 
> > in the lookup in case they're from different scopes.  I guess to avoid that 
> > we'd have to do the filtering immediately when we collect the declarations 
> > from a particular DC.
> I think that delaying the removal until after the main loop would just 
> complicate things, as then in the main loop we would have to check each index 
> to see if it's something we're going to later remove. I can adjust it to do 
> the erasing more like it's done in the main loop though, i.e. move the erased 
> element to the end and decrement N, so the call to Decls.truncate will remove 
> it. We can't use bitset though, as that takes the size of the bitset (which 
> in this case would be the number of decls) as a template parameter.
llvm::BitVector should work for this. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154503/new/

https://reviews.llvm.org/D154503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154502: [AST] Fix bug in UnresolvedSet::erase of last element

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154502/new/

https://reviews.llvm.org/D154502

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added subscribers: iana, ributzka.
dexonsmith added a comment.

In D103930#4310061 , @ivanmurashko 
wrote:

> Friendly ping
>
> @arphaman, @jansvoboda11, I have made the patch buildable on all platforms 
> and have all tests passed. There was also a small fix (temp path for modules 
> artefact) at the test that could fix its run on some platforms. Could you 
> look at it? Does it have any issues on your side?

Alternatively, maybe @vsapsai, @ributzka, or @iana...?

Given that header maps are somewhat Apple-specific and unit test coverage is a 
bit lacking for these sorts of interactions, it'd be nice for someone to check 
this with Apple-internal stuff. But if you're okay with it landing, and then 
you triage internal issues not covered by public tests later, please say that 
as well!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103930/new/

https://reviews.llvm.org/D103930

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D83906#4186887 , @hoy wrote:

> In D83906#4184916 , @dexonsmith 
> wrote:
>
>> In D83906#4183453 , @hoy wrote:
>>
>>> Wondering if we can come up with a way to tell the optimizer about that, 
>>> e.g., through a new module flag. When it comes to LTO, the selection of 
>>> linkonce_odr symbols should already been done and the optimizer may be able 
>>> to recompute the attributes based on pre-LTO attributes, or at least we can 
>>> allow IPO to one module only, which should still do a better job than FE 
>>> does?
>>
>> I don't think there's much point in passing anything to LTO. There are very 
>> few `linkonce_odr` symbols in LTO, since LTO has the advantage of an export 
>> list from the link. Symbols not on the export list are internalized (they're 
>> given local linkage).
>
> That sounds to me an opportunity to get a broader IPO done precisely in the 
> prelink optimizer, as long as we find a way to tell it the incoming IR has 
> source fidelity. What do you think about idea of introducing a module flag? 
> Maybe it's worth discussing in the forum as a followup of introducing a cc1 
> flag for a stable IR gen.

I'm not sure I'm following.

The prelink optimizer will already be internalizing (i.e., NOT exporting) these 
symbols. That should be enough. AFAICT, it's non-LTO pipelines that might have 
headroom after this is reverted.

I'm also not sure what the module flag would be for. If "this module has source 
fidelity", it won't work, because the gadgets I'm aware of are implemented in 
function passes (probably `-instcombine`?). A function pass isn't allowed to 
touch module state. Were you thinking of a different module flag? (But, I 
repeat, I think LTO pipelines have nothing to worry about anyway.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83906/new/

https://reviews.llvm.org/D83906

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D83906#4183453 , @hoy wrote:

> Wondering if we can come up with a way to tell the optimizer about that, 
> e.g., through a new module flag. When it comes to LTO, the selection of 
> linkonce_odr symbols should already been done and the optimizer may be able 
> to recompute the attributes based on pre-LTO attributes, or at least we can 
> allow IPO to one module only, which should still do a better job than FE does?

I don't think there's much point in passing anything to LTO. There are very few 
`linkonce_odr` symbols in LTO, since LTO has the advantage of an export list 
from the link. Symbols not on the export list are internalized (they're given 
local linkage).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83906/new/

https://reviews.llvm.org/D83906

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a subscriber: sanjoy.
dexonsmith added a comment.

In D83906#4182902 , @rjmccall wrote:

> So your argument is that it would not be possible to recognize that we're 
> doing such an optimization and mark the function as having had a possible 
> semantics change?

I suspect it would be error-prone to do that precisely. I'd bet there are a 
variety of hard-to-reason about examples. Originally, when @sanjoy was first 
describing this problem (to me and others), his examples all had UB in the 
original code (e.g., reading and writing to globals in different threads). 
Eventually he invented the adjacent-atomic-load device, described above, which 
does not rely on UB in the original code. I just assume there are more devices 
out there that we don't know about or understand.

Maybe it would be useful to do it imprecisely? E.g, have all transformation 
passes mark all functions as changed (or drop a `pristine` attribute)? Then at 
least you know whether it has come directly from IRGen.

Not sure if it's valuable enough though. I don't think the regressions were as 
bad as we expected. It seems okay for the optimizer to delay propagating 
attributes from de-refineable functions until you have the export list for the 
link (and non-expected symbols can be internalized).



Sorry... this seems to be pretty off topic, in a way, although maybe not many 
people know the de-refinement ins and outs so maybe this is useful.

My original points:

- Moving this to the middle end isn't easy (currently, it doesn't have the info 
it needs, although John might have a proposal for providing it)
- Dropping frontend peepholes in favour of stable IR output seems novel and 
might deserve a forum discussion (I'd be in favour, personally)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83906/new/

https://reviews.llvm.org/D83906

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D83906#4182847 , @hoy wrote:

> As far as I know, the optimizer IPO pass that infers function attributes 
> (i..e `InferFunctionAttrsPass`) is placed at the very beginning of the 
> optimization pipeline.  Does this sound to you that the side effects computed 
> for linkonce_odr functions there can be trusted by the rest of the pipeline?

Depends what you mean by "trusted". It assumes the attributes accurately 
describe the function it sees. The properties promised there will apply if/when 
the code is inlined. But, since the commit in 2016, it doesn't trust that they 
fully describe the source semantics, so IPA ignores them when the function is 
not inlined.

Note that the optimizer doesn't know if its input IR has already been 
optimized. Is this the first optimizer that has run on the IR, or could side 
effects have been refined away already? E.g., if the optimization pipeline in 
question runs at LTO time, the compile-time optimization pipeline has already 
run.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83906/new/

https://reviews.llvm.org/D83906

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D83906#4182777 , @rjmccall wrote:

> In D83906#4182287 , @dexonsmith 
> wrote:
>
>> - At IRGen time, you know the LLVM attributes have not been adjusted after 
>> the optimized refined the function's behaviour. It should be safe to have 
>> IPA peepholes, as long as IRGen's other peepholes don't refine behaviour and 
>> add attributes based on that.
>> - In the optimizer, if you're looking at de-refineable function then you 
>> don't know which attributes come directly from the source and which were 
>> implied by optimizer refinements. You can't trust you'll get the same 
>> function attributes at runtime.
>
> Hmm.  I see what you're saying, but it's an interesting question how it 
> applies here.  In principle, the optimizer should not be changing the 
> observable semantics of functions, which certainly includes things like 
> whether the function throws.  Maybe the optimizer can only figure out that a 
> function throws in one TU, but if it "figures that out" and then a function 
> with supposedly the same semantics actually does throw — not just retains the 
> static ability to throw on a path that happens not to be taken dynamically, 
> but actually throws at runtime — then arguably something has gone badly wrong.

I believe in my example, it's kind of the reverse. Only one TU *remembers* that 
the function can throw; the other one "forgets" because it has optimized its 
variant not to `throw`.

Maybe it's useful to note that, while `maybe_nounwind` has no UB, whether it 
throws or not depends on thread timing, and is generally non-reproducible (run 
it twice, you can get different results). In the TU that forgets, the optimizer 
is choosing to assume that the two adjacent atomic loads happen so quickly that 
no store happens in between; choosing the thread timing where there's no store 
to contend with. This is a valid refinement of the original source semantics -- 
optimizers are allowed to CSE adjacent atomic loads.

> As I recall, the de-refinement discussion was originally about properties 
> that are *not* invariant to optimization in this way, things like whether the 
> function uses one of its arguments.  Those properties are not generally 
> considered to be part of the function's externally-observable semantics.

The example described in the referenced de-refinement commit is where a 
function that writes to memory is refined to `readnone`. I think my `nounwind` 
example above is analogous.

Here's the original from https://reviews.llvm.org/D18634:

> For instance, FunctionAttrs cannot assume a comdat function is
> actually readnone even if it does not have any loads or stores in
> it; since there may have been loads and stores in the "original
> function" that were refined out in the currently visible variant, and
> at the link step the linker may in fact choose an implementation with
> a load or a store. As an example, consider a function that does two
> atomic loads from the same memory location, and writes to memory only
> if the two values are not equal. The optimizer is allowed to refine
> this function by first CSE'ing the two loads, and the folding the
> comparision to always report that the two values are equal. Such a
> refined variant will look like it is readonly. However, the
> unoptimized version of the function can still write to memory (since
> the two loads can result in different values), and selecting the
> unoptimized version at link time will retroactively invalidate
> transforms we may have done under the assumption that the function
> does not write to memory.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83906/new/

https://reviews.llvm.org/D83906

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D83906#4182428 , @hoy wrote:

> In D83906#4182287 , @dexonsmith 
> wrote:
>
>> In C++, you get linkonce_odr all over the place. It's basically all 
>> functions that are defined in C++ headers that are available for inlining.
>
>
>
>> On the other hand, the frontend knows the token sequence from the source 
>> language. It knows whether function B is inherently nounwind based on its 
>> ODR token sequence; in which case, it's safe to use the attribute for an IPA 
>> peephole.
>
> Thanks for the detailed explanation again! As you pointed out previously, 
> linkonce_odr is something the front end can optimize. I'm wondering why the 
> front end is confident about that the linker would not replace the current 
> definition with something else.

The frontend has generated unrefined IR with all side effects from the 
must-be-ODR-equivalent source still present. It's not until on optimizer gets 
at it that side effects can be refined away. (Unless the IRGen peepholes are 
powerful enough to refine away side effects, but I don't believe IRGen does 
that.)

Since the IR from IRGen is unrefined (still has all side effects present in the 
source), whatever the linker/loader chooses cannot gain "extra" side effects 
through de-refinement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83906/new/

https://reviews.llvm.org/D83906

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D83906#4181981 , @hoy wrote:

> That said, the LLVM optimizer does not strictly subsume the front-end because 
> of how it fails to handle `linkonce_odr` functions as in 
> https://reviews.llvm.org/D18634. I'm wondering how common the `linkonce_odr` 
> linkage is for C++. In @wlei's example, none of the functions there is 
> `linkonce_odr`. Is there a particular source-level annotate that specifies 
> functions to be `linkonce_odr`?

In C++, you get `linkonce_odr` all over the place. It's basically all functions 
that are defined in C++ headers that are available for inlining.

- any function marked `inline`
- any function in a class/struct whose declaration is its definition 
(approximately all templated code)

A few exceptions:

- If a function is explicitly instantiated (e.g., member functions of `T` 
if `template class T;`), it gets `weak_odr`, which IIRC cannot be 
de-refined?
- If a function has local linkage (like free functions with `static inline`), 
it gets `internal`, which cannot be de-refined.
- If a function is marked `inline` inside an `extern "C"` block, it gets 
`available_externally`. This can also be de-refined (but without ODR, you 
wouldn't be tempted to optimize based on its attributes anyway).



> It sounds to me that at link time only equivalent symbols can replace each 
> other. Then de-refining some of those equivalent symbols should not affect 
> their semantics as far as nothrow is concerned? Just as @rjmccall pointed 
> out, the C++ language guarantee we're starting from is that the source 
> semantics of all versions of the function are identical.

The rule is subtly different. Only symbols that are source-equivalent can 
replace each other. But they aren't necessarily equivalent to the function you 
see, which may have been refined by optimization.

Here's a concrete example. Say we have function `maybe_nounwind` that is not 
`nounwind` at the source level, and a `catch_all` function that wraps it.

  // Defined in header.
  extern std::atomic Global;
  
  // LLVM: linkonce_odr
  inline int maybe_nounwind(int In) {
int Read1 = Global;
int Read2 = Global;
if (Read1 != Read2)
  throw 0;
  
return /* Big, non-inlineable computation on In */;
  }
  
  // Defined in source.
  // LLVM: nounwind
  int catch_all(int In) {
try {
  return maybe_nounwind(In);
} catch (...) {
  return -1;
}
  }

There's no UB here, since comparing two atomic loads is allowed. In rare cases, 
an unoptimized `maybe_nounwind` could throw, if another thread is changing the 
value of `Global` between the two loads.

But the optimizer will probably CSE the two atomic loads since it's allowed to 
assume that both loads happen at the same time. This refines `maybe_nounwind`. 
It'll turn into IR equivalent to:

  // Defined in header. Then optimized.
  // LLVM: linkonce_odr nounwind readnone
  inline int maybe_nounwind(int In) {
return /* Big, non-inlineable computation on In */;
  }
  
  // Defined in source. Then optimized.
  // LLVM: nounwind
  int catch_all(int In) {
try {
  return maybe_nounwind(In);
} catch (...) {
  return -1;
}
  }

It's important that `catch_all` is NOT optimized based on `maybe_nounwind`'s 
new `nounwind` attribute. At link time, it's possible for the linker to choose 
an unoptimized copy of `maybe_nounwind`. Just in case it does, `catch_all` 
needs to keep its `try/catch` block, since unoptimized `maybe_nounwind` can 
throw. Similarly, `catch_all` should not be marked `readnone`, even though the 
refined/optimized `maybe_nounwind` is `readnone`, since a de-refined copy reads 
from memory.

In D83906#4181876 , @rjmccall wrote:

> There are *some* properties we can still assume about `linkonce_odr` 
> functions despite them being replaceable at link time.  The high-level 
> language guarantee we're starting from is that the source semantics of all 
> versions of the function are identical.  The version of the function we're 
> looking at has been transformed from the original source — it is, after all, 
> now LLVM IR, not C/C++ — but it has presumably faithfully preserved the 
> source semantics.  We can therefore rely on any properties of the semantics 
> that are required to be preserved by transformation, which includes things 
> like "does it terminate", "what value does it return", "what side effects 
> does it perform", and so on.  What we can't rely on are properties of the 
> implementation that are not required to be preserved by transformation, like 
> whether or not it uses a certain argument — transformations are permitted to 
> change that.



- At IRGen time, you know the LLVM attributes have not been adjusted after the 
optimized refined the function's behaviour. It should be safe to have IPA 
peepholes, as long as IRGen's other peepholes don't refine behaviour and add 
attributes based on that.

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/tools/libclang/CXType.cpp:13
 
+#include "CXType.h"
 #include "CIndexer.h"

vedgy wrote:
> collinbaker wrote:
> > vedgy wrote:
> > > I guess //clang-format// did this include reordering. But it certainly 
> > > looks out of place and the include order becomes wrong. So I think it 
> > > should be reverted.
> > I don't agree, it's pretty standard for a source file to have its 
> > associated header include at the top.
> You are right, I haven't realized the header-source association. The diff is 
> still unrelated to the patch. But I'm no longer sure what's right, so won't 
> insist on anything.
This is correct behaviour from clang-format.

Given that there were no functional changes to includes, typically we’d omit 
clang-format cleanups. (I know there was a change below originally, but that 
was reverted.)

I’m fine leaving the header clean-up in, or separating it out to different NFC 
commit (ideal; could be done when pushing), or skipping it entirely. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Oh, de-refining is pretty nifty / evil. This patch has background:
https://reviews.llvm.org/D18634

Since 2016, the optimizer is not allowed to do IPA on functions that can be 
de-refined (such as `linkonce_odr` functions).

Here's a hypothetical problematic scenario for the optimizer:

- original IR for B has a `throw` somewhere
- function A invokes function B
- in this TU, B is optimized and removes exceptions, and gets marked `nounwind`
- function A leverages the `nounwind` to turn the `invoke` into a `call`
- function B is de-refined at link/load time: linker chooses a *different* 
function B which still has a `throw`
- "evil magic" happens (see the discussions around the patch linked above)
- a crash is introduced

At first blush, it sounds like this could only be a problem if the code has UB 
in it. However, read the above patch (and follow-ups, and related discussion) 
for a variety of examples of non-UB cases where IPA on de-refineable functions 
introduces crashes. I don't know for sure this could be a problem for 
`nounwind` specifically, but in general the LLVM optimizer doesn't look at 
attributes of de-refineable functions.

(Note that if you're doing LTO (like AutoFDO), this usually won't block 
optimization, since at LTO time there are very few de-refineable functions 
(most `linkonce_odr` functions are internalized, and not exported as weak). So 
if we added a `-cc1` flag to prefer "stable IR" over "frontend peepholes", it 
would make sense for `-flto` to imply it.)

On the other hand, the frontend knows the token sequence from the source 
language. It knows whether function `B` is inherently `nounwind` based on its 
ODR token sequence; in which case, it's safe to use the attribute for an IPA 
peephole.



BTW, I'm not personally against removing this peephole from the frontend (even 
without a flag), or limiting it somehow to cases where it doesn't make IR 
output unstable. I like the idea of stable IRGen output.

Nevertheless, it feels like removing IPA-based peepholes from Clang in the name 
of stable IRGen output is a change in direction, which might deserve a 
discussion in the forums rather than in a particular patch review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83906/new/

https://reviews.llvm.org/D83906

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D83906#4179512 , @wlei wrote:

> Hi @ahatanak
>
> We recently hit an issue of inconsistent codegen related with this 
> optimization. In one build, Clang frontend generates different llvm IRs for 
> the same function that is originally from one header file. It turned out this 
> optimization gives different results for different function definition order 
> which is naturally unstable.
>
> See this two repro programs:
>
> p1.cpp: https://godbolt.org/z/bavTYEG1x
>
>   void foo() {};
>   void bar() noexcept {foo();};
>
> 
p2.cpp: https://godbolt.org/z/zfsnzPrE6
>
>   void foo();
>   void bar() noexcept {foo();};
>   void foo(){};
>
> See the codegens of bar are different, for p2.cpp, the callee(foo)’s 
> definition is after the caller(bar), it's unknown to be marked `nounwind` 
> before it see foo's definition, so it still generates the `invoke` things.
>
> This inconsistency affected the AutoFDO, one of our work assigns consecutive 
> number IDs to the BBs of CFG, the unstable CFGs causes the BB ID mismatched 
> and a lot of samples are lost.
>
> Would like to hear from your feedback. Wondering if FE can handle this 
> perfectly or perhaps we can just leave it for BE. Thank you in advance!
>
> cc @hoy @modimo @wenlei

To be clear, there's no miscompile, correct?

(Also, can the backend safely optimize an `invoke` to a `linkonce_odr` function 
that's `nounwind` to a `call`? I thought it couldn't, in case the function is 
de-refined to a version that's not `nounwind`. But the frontend can do it since 
it has access to the source and knows it can't be de-refined in that way?)

In any case, let's say the backend can do this optimization.

I wonder if this is just a single example, where there could be various other 
(header-related) peepholes that cause similar problems for stable output. IIRC, 
the usual Clang approach is to make as-close-to-optimal IR up front, but maybe 
in some situations it's desirable to delay optimizations to improve stability. 
Another application where that could be useful is caching.

Maybe the high level principle deserves a broader discussion on the forums. Do 
we want IRGen to prefer stable IR, or optimized IR? Should there be a `-cc1` 
flag to decide (which AutoFDO could set)?

@rjmccall, any thoughts?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83906/new/

https://reviews.llvm.org/D83906

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D130303#4175664 , @collinbaker 
wrote:

> @dexonsmith can you weigh in?

Introducing `clang_isBitFieldDecl` sounds like a clean/straightforward solution 
to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

In D130303#3724392 , @dexonsmith 
wrote:

> In D130303#3724247 , @rnk wrote:
>
>> Pinging alternative reviewer +@dexonsmith for a libclang API addition
>
> Looks reasonable to me -- this only changes behaviour of the existing API 
> when there was corruption before -- but if the goal is to get a vendor of 
> libclang-as-a-stable-API to sign off, I can't help.
>
> @arphaman, if you're busy, is there someone else that could take a quick look?

This still LGTM, for the same reasons. Let's just ship this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:6343
  "Invalid data, missing pragma diagnostic states");
-  SourceLocation Loc = ReadSourceLocation(F, Record[Idx++]);
-  auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc);
-  assert(IDAndOffset.first.isValid() && "invalid FileID for transition");
-  assert(IDAndOffset.second == 0 && "not a start location for a FileID");
+  FileID FID = ReadFileID(F, Record, Idx);
+  assert(FID.isValid() && "invalid FileID for transition");

alexfh wrote:
> alexfh wrote:
> > dexonsmith wrote:
> > > alexfh wrote:
> > > > alexfh wrote:
> > > > > jansvoboda11 wrote:
> > > > > > alexfh wrote:
> > > > > > > dexonsmith wrote:
> > > > > > > > eaeltsin wrote:
> > > > > > > > > This doesn't work as before, likely because ReadFileID 
> > > > > > > > > doesn't do TranslateSourceLocation.
> > > > > > > > > 
> > > > > > > > > Our tests fail.
> > > > > > > > > 
> > > > > > > > > I tried calling TranslateSourceLocation here and the tests 
> > > > > > > > > passed:
> > > > > > > > > ```
> > > > > > > > >   SourceLocation Loc = 
> > > > > > > > > Diag.SourceMgr->getComposedLoc(FID, 0);
> > > > > > > > >   SourceLocation Loc2 = TranslateSourceLocation(F, Loc);
> > > > > > > > >   auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc2);
> > > > > > > > > 
> > > > > > > > >   // Note that we don't need to set up 
> > > > > > > > > Parent/ParentOffset here, because
> > > > > > > > >   // we won't be changing the diagnostic state within 
> > > > > > > > > imported FileIDs
> > > > > > > > >   // (other than perhaps appending to the main source 
> > > > > > > > > file, which has no
> > > > > > > > >   // parent).
> > > > > > > > >   auto  = Diag.DiagStatesByLoc.Files[IDAndOffset.first];
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > Sorry I don't know the codebase, so this fix is definitely 
> > > > > > > > > ugly :) But it shows the problem.
> > > > > > > > > 
> > > > > > > > I don't think that's the issue, since `ReadFileID()` calls 
> > > > > > > > `TranslateFileID`, which should seems like it should be 
> > > > > > > > equivalent.
> > > > > > > > 
> > > > > > > > However, I notice that the post-increment for `Idx` got 
> > > > > > > > dropped! Can you try replacing the line of code with the 
> > > > > > > > following and see if that fixes your tests (without any extra 
> > > > > > > > TranslateSourceLocation logic)?
> > > > > > > > ```
> > > > > > > > lang=c++
> > > > > > > > FileID FID = ReadFileID(F, Record, Idx++);
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > If so, maybe you can contribute that fix with a reduced 
> > > > > > > > testcase? I suggest adding me, @vsapsai, @Bigcheese, and 
> > > > > > > > @jansvoboda11 as reviewers.
> > > > > > > > 
> > > > > > > > @alexfh, maybe you can check if this fixes your tests as well?
> > > > > > > > 
> > > > > > > > (If this is the issue, it's a bit surprising we don't have 
> > > > > > > > existing tests covering this case... and embarrassing I missed 
> > > > > > > > it when reviewing initially!)
> > > > > > > I've noticed the dropped `Idx` post-increment as well, but I went 
> > > > > > > a step further and looked at the `ReadFileID` implementation, 
> > > > > > > which is actually doing a post-increment itself, and accepts 
> > > > > > > `Idx` by reference:
> > > > > > > ```
> > > > > > >   FileID ReadFileID(ModuleFile , const RecordDataImpl ,
> > > > > > > unsigned ) const {
> > > > > > > return TranslateFileID(F, FileID::get(Record[Idx++]));
> > > > > > >   }
> > > > > > > ```
> > > > > > > 
> > > > > > > Thus, it seems to be correct. But what @eaeltsin  has found is 
> > > > > > > actually a problem for us.  I'm currently trying to make an 
> > > > > > > isolated test case, but it's quite tricky (as header modules are 
> > > > > > > =\). It may be the case that our build setup relies on something 
> > > > > > > clang doesn't explicitly promises, but the fact is that the 
> > > > > > > behavior (as observed by our build setup) has changed. I'll try 
> > > > > > > to revert the commit for now to get us unblocked and provide a 
> > > > > > > test case as soon as I can.
> > > > > > Thanks for helping out @dexonsmith, we did have the week off.
> > > > > > 
> > > > > > @eaeltsin @alexfhDone, are you able to provide the failing test 
> > > > > > case? I'm happy to look into it and re-land this with a fix.
> > > > > I've spent multiple hours trying to extract an observable test case. 
> > > > > It turned out to be too hairy of a yaq to shave: for each compilation 
> > > > > a separate sandboxed environment is created with a separate symlink 
> > > > > tree with just the inputs necessary for that action. Some of the 
> > > > > inputs are prebuilt module files (e.g. for libc++) that are 
> > > > > version-locked with the compiler. So far @jgorbe and I could reduce 
> > > > > 

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:6343
  "Invalid data, missing pragma diagnostic states");
-  SourceLocation Loc = ReadSourceLocation(F, Record[Idx++]);
-  auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc);
-  assert(IDAndOffset.first.isValid() && "invalid FileID for transition");
-  assert(IDAndOffset.second == 0 && "not a start location for a FileID");
+  FileID FID = ReadFileID(F, Record, Idx);
+  assert(FID.isValid() && "invalid FileID for transition");

alexfh wrote:
> alexfh wrote:
> > jansvoboda11 wrote:
> > > alexfh wrote:
> > > > dexonsmith wrote:
> > > > > eaeltsin wrote:
> > > > > > This doesn't work as before, likely because ReadFileID doesn't do 
> > > > > > TranslateSourceLocation.
> > > > > > 
> > > > > > Our tests fail.
> > > > > > 
> > > > > > I tried calling TranslateSourceLocation here and the tests passed:
> > > > > > ```
> > > > > >   SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FID, 0);
> > > > > >   SourceLocation Loc2 = TranslateSourceLocation(F, Loc);
> > > > > >   auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc2);
> > > > > > 
> > > > > >   // Note that we don't need to set up Parent/ParentOffset 
> > > > > > here, because
> > > > > >   // we won't be changing the diagnostic state within imported 
> > > > > > FileIDs
> > > > > >   // (other than perhaps appending to the main source file, 
> > > > > > which has no
> > > > > >   // parent).
> > > > > >   auto  = Diag.DiagStatesByLoc.Files[IDAndOffset.first];
> > > > > > ```
> > > > > > 
> > > > > > Sorry I don't know the codebase, so this fix is definitely ugly :) 
> > > > > > But it shows the problem.
> > > > > > 
> > > > > I don't think that's the issue, since `ReadFileID()` calls 
> > > > > `TranslateFileID`, which should seems like it should be equivalent.
> > > > > 
> > > > > However, I notice that the post-increment for `Idx` got dropped! Can 
> > > > > you try replacing the line of code with the following and see if that 
> > > > > fixes your tests (without any extra TranslateSourceLocation logic)?
> > > > > ```
> > > > > lang=c++
> > > > > FileID FID = ReadFileID(F, Record, Idx++);
> > > > > ```
> > > > > 
> > > > > If so, maybe you can contribute that fix with a reduced testcase? I 
> > > > > suggest adding me, @vsapsai, @Bigcheese, and @jansvoboda11 as 
> > > > > reviewers.
> > > > > 
> > > > > @alexfh, maybe you can check if this fixes your tests as well?
> > > > > 
> > > > > (If this is the issue, it's a bit surprising we don't have existing 
> > > > > tests covering this case... and embarrassing I missed it when 
> > > > > reviewing initially!)
> > > > I've noticed the dropped `Idx` post-increment as well, but I went a 
> > > > step further and looked at the `ReadFileID` implementation, which is 
> > > > actually doing a post-increment itself, and accepts `Idx` by reference:
> > > > ```
> > > >   FileID ReadFileID(ModuleFile , const RecordDataImpl ,
> > > > unsigned ) const {
> > > > return TranslateFileID(F, FileID::get(Record[Idx++]));
> > > >   }
> > > > ```
> > > > 
> > > > Thus, it seems to be correct. But what @eaeltsin  has found is actually 
> > > > a problem for us.  I'm currently trying to make an isolated test case, 
> > > > but it's quite tricky (as header modules are =\). It may be the case 
> > > > that our build setup relies on something clang doesn't explicitly 
> > > > promises, but the fact is that the behavior (as observed by our build 
> > > > setup) has changed. I'll try to revert the commit for now to get us 
> > > > unblocked and provide a test case as soon as I can.
> > > Thanks for helping out @dexonsmith, we did have the week off.
> > > 
> > > @eaeltsin @alexfhDone, are you able to provide the failing test case? I'm 
> > > happy to look into it and re-land this with a fix.
> > I've spent multiple hours trying to extract an observable test case. It 
> > turned out to be too hairy of a yaq to shave: for each compilation a 
> > separate sandboxed environment is created with a separate symlink tree with 
> > just the inputs necessary for that action. Some of the inputs are prebuilt 
> > module files (e.g. for libc++) that are version-locked with the compiler. 
> > So far @jgorbe and I could reduce this to four compilation steps with their 
> > own symlink trees with inputs. While I could figure out some of the factors 
> > that affect reproducibility (for example, symlinks are important, since 
> > making a deep copy of the input directories makes the issue disappear), it 
> > will take a few more hours of concentrated yak shaving to bring this to a 
> > shareable state. I'm not sure I have much more time to sink into 
> > investigating this. 
> > 
> > It seems like examining code based on @eaeltsin's finding may be a more 
> > fruitful path to synthesizing a regression test. Could you try following 
> > that 

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I just realized @jansvoboda11 is probably out on holiday this week (IIRC, Apple 
usually gets this week off). Since this was committed almost a month ago, I'm 
guessing this isn't enough of a blocker that we need to revert rather than wait 
until next week (and there are some commits on top that rely on this!). But 
probably reverting the stack would be an option if it's critical.

In the meantime, I'll try to help.




Comment at: clang/lib/Serialization/ASTReader.cpp:6343
  "Invalid data, missing pragma diagnostic states");
-  SourceLocation Loc = ReadSourceLocation(F, Record[Idx++]);
-  auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc);
-  assert(IDAndOffset.first.isValid() && "invalid FileID for transition");
-  assert(IDAndOffset.second == 0 && "not a start location for a FileID");
+  FileID FID = ReadFileID(F, Record, Idx);
+  assert(FID.isValid() && "invalid FileID for transition");

eaeltsin wrote:
> This doesn't work as before, likely because ReadFileID doesn't do 
> TranslateSourceLocation.
> 
> Our tests fail.
> 
> I tried calling TranslateSourceLocation here and the tests passed:
> ```
>   SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FID, 0);
>   SourceLocation Loc2 = TranslateSourceLocation(F, Loc);
>   auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc2);
> 
>   // Note that we don't need to set up Parent/ParentOffset here, because
>   // we won't be changing the diagnostic state within imported FileIDs
>   // (other than perhaps appending to the main source file, which has no
>   // parent).
>   auto  = Diag.DiagStatesByLoc.Files[IDAndOffset.first];
> ```
> 
> Sorry I don't know the codebase, so this fix is definitely ugly :) But it 
> shows the problem.
> 
I don't think that's the issue, since `ReadFileID()` calls `TranslateFileID`, 
which should seems like it should be equivalent.

However, I notice that the post-increment for `Idx` got dropped! Can you try 
replacing the line of code with the following and see if that fixes your tests 
(without any extra TranslateSourceLocation logic)?
```
lang=c++
FileID FID = ReadFileID(F, Record, Idx++);
```

If so, maybe you can contribute that fix with a reduced testcase? I suggest 
adding me, @vsapsai, @Bigcheese, and @jansvoboda11 as reviewers.

@alexfh, maybe you can check if this fixes your tests as well?

(If this is the issue, it's a bit surprising we don't have existing tests 
covering this case... and embarrassing I missed it when reviewing initially!)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137213/new/

https://reviews.llvm.org/D137213

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a subscriber: rnk.
dexonsmith added inline comments.



Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878
+  // with native path seperator, regardless of the actual path seperator
+  // used in YAMLFilePath field.
+#ifndef _WIN32

bnbarham wrote:
> `separator` -> `separator`
> 
> As per my comment above, this is only true because we're either using posix 
> or windows_backslash. I'm not sure what we really want here, should we always 
> convert to native? Any thoughts @dexonsmith?
> 
> FWIW the non-windows case is identical to the above block. It'd be 
> interesting to test `/` and `\` on both filesystems.
I think we might need a Windows person to help answer; @rnk, can you suggest 
someone to help answer @bnbarham's question?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137473/new/

https://reviews.llvm.org/D137473

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D137473#3915497 , @bnbarham wrote:

> This seems reasonable to me in general. @dexonsmith in case you have any 
> thoughts.

SGTM! (I haven't reviewed in detail but I figure @bnbarham is on it...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137473/new/

https://reviews.llvm.org/D137473

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/include/clang/Basic/SourceManager.h:135-139
   /// References the file which the contents were actually loaded from.
   ///
   /// Can be different from 'Entry' if we overridden the contents of one file
   /// with the contents of another file.
   const FileEntry *ContentsEntry;

benlangmuir wrote:
> dexonsmith wrote:
> > It's possible this could be factored out as a follow-up, and/or moved up to 
> > the Named ContentCache level. (Not sure... asking a question really...)
> I think this belongs here. It is used to lazily get the contents of the file 
> (see the getBuffer functions).
You're right. I wonder what if that laziness is inherently load-bearing. Seems 
odd that clang would want to create a FileID without very quickly trying to 
open it. But maybe that's important somewhere/somehow. Certainly off-topic for 
this patch though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137304/new/

https://reviews.llvm.org/D137304

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/include/clang/Basic/SourceManager.h:261-263
+  /// FIXME: Make non-optional using a virtual file as needed, remove \c
+  /// Filename and use \c OrigEntry.getNameAsRequested() instead.
+  OptionalFileEntryRefDegradesToFileEntryPtr OrigEntry;

benlangmuir wrote:
> dexonsmith wrote:
> > I think this patch should (or does?) implement this FIXME.
> I think it can still be None/nullptr for some buffers.  I'll verify that.  
> IIRC the trick to removing Filename is to create virtual file refs in some 
> places that currently don't have one.
Oh, maybe you're right. But if you're able to implement, it might help pay for 
some of the memory overhead of your patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137304/new/

https://reviews.llvm.org/D137304

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/include/clang/Basic/SourceManager.h:652-662
+  llvm::DenseMap
+  NamedFileInfos;
+
   /// Memoized information about all of the files tracked by this
   /// SourceManager.
   ///

It feels expensive to have both of these maps. I wonder if instead we could do 
something like:

```
lang=c++
DenseMap> FirstFileInfo;
```
I.e., lookup by `FileEntry*`, then linear search for the right `FileEntryRef`.

Similar idea: just point at `NamedContentCache*` directly here, but add 
`NamedContentCache *NamedContentCache::Next` to turn it into a linked list. 
Also lookup by `FileEntry*` and then a linear search for the right ref.

Note that `FileInfo` itself would have a direct pointer to the right thing; no 
need for a linear search.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137304/new/

https://reviews.llvm.org/D137304

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/include/clang/Basic/SourceManager.h:135-139
   /// References the file which the contents were actually loaded from.
   ///
   /// Can be different from 'Entry' if we overridden the contents of one file
   /// with the contents of another file.
   const FileEntry *ContentsEntry;

It's possible this could be factored out as a follow-up, and/or moved up to the 
Named ContentCache level. (Not sure... asking a question really...)



Comment at: clang/include/clang/Basic/SourceManager.h:147-163
   /// Indicates whether the buffer itself was provided to override
   /// the actual file contents.
   ///
   /// When true, the original entry may be a virtual file that does not
   /// exist.
   unsigned BufferOverridden : 1;
 

Have you thought about whether it makes sense for these fields to be shared for 
all `FileEntryRef`s to the same `FileEntry`?  (Maybe it does! just checking)



Comment at: clang/include/clang/Basic/SourceManager.h:261-263
+  /// FIXME: Make non-optional using a virtual file as needed, remove \c
+  /// Filename and use \c OrigEntry.getNameAsRequested() instead.
+  OptionalFileEntryRefDegradesToFileEntryPtr OrigEntry;

I think this patch should (or does?) implement this FIXME.



Comment at: clang/include/clang/Basic/SourceManager.h:265-268
+  /// The filename that is used to access OrigEntry.
+  ///
+  /// FIXME: Remove this once OrigEntry is a FileEntryRef with a stable name.
+  StringRef Filename;

I think you can delete this field. You're effectively implementing the FIXME.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137304/new/

https://reviews.llvm.org/D137304

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

> Consider making the FileEntryRef changes here the default -- it doesn't make 
> sense to me that FileEntryRef == or DenseMap would match FileEntry pointer 
> semantics instead of filename semantics.

Yeah, that was something I added, and I agree it's unfortunate / hard to reason 
about. It was a concession to make it easier to replace `FileEntry*` with 
`FileEntryRef` seamlessly in lots of places with NFC; the logic is "we want 
only one entry FileEntry in this map; let the first one win". Maybe most places 
would prefer a functionality change though, and, either way, the DenseMaps that 
need this unique-by-`FileEntry*` behaviour should probably be the ones using a 
custom DenseMapInfo.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137304/new/

https://reviews.llvm.org/D137304

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, with one suggestion for the test inline.




Comment at: clang/test/Modules/add-remove-irrelevant-module-map.m:22
+// Build modules with the non-affecting "a/module.modulemap".
+// RUN: %clang_cc1 -I %t/a -I %t/b -I %t/c -I %t -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash 
%t/test-simple.m
+// RUN: mv %t/cache %t/cache-with

Maybe the CC1s should add `-verify` and `test-simple.m` should have `// 
expected-no-diagnostics` to help protect against bitrot?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137216: [clang][modules] NFCI: Avoid unnecessary serialization logic for non-affecting files

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137216/new/

https://reviews.llvm.org/D137216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, although I there's format-is-probably-compatible-version as a courtesy 
for tooling, does that need a bump here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137213/new/

https://reviews.llvm.org/D137213

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137211: [clang][modules] NFCI: Unify FileID writing/reading

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137211/new/

https://reviews.llvm.org/D137211

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137214: [clang][modules] NFCI: Scaffolding for serialization of adjusted SourceManager offsets

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137214/new/

https://reviews.llvm.org/D137214

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137215: [clang] NFC: Extract lower-level SourceManager functions

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137215/new/

https://reviews.llvm.org/D137215

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D136624#3900593 , @jansvoboda11 
wrote:

> Ah, I forgot to mention this. Building the modules is now only 0.2% slower 
> and importing them 1.2% faster (compared to PCMs with all input files 
> serialized).

Awesome. All upside then :).

I added a few nitpick-y comments inline. I can take another look once you've 
made the PCMs bit-for-bit identical and updated the test (is that happening 
here, or in a separate review)?




Comment at: clang/include/clang/Basic/SourceManager.h:1831-1833
+  bool isLoadedOffset(SourceLocation::UIntTy SLocOffset) const {
+return SLocOffset >= CurrentLoadedOffset;
+  }

jansvoboda11 wrote:
> dexonsmith wrote:
> > The logic for `isLoadedOffset()` suggests that it could maybe be subsumed 
> > with "location past the end"?
> I don't think so - we don't want to adjust loaded offsets. Their invariant is 
> that they grow from 2^31 downwards.
> 
> We do want to adjust local offsets past the last non-affecting file though.
Oops, right!



Comment at: clang/lib/Serialization/ASTWriter.cpp:4538
+  for (unsigned I = 1; I != N; ++I) {
+// Get this source location entry.
+const SrcMgr::SLocEntry *SLoc = (I);

Not sure this comment adds much on top of the code on the next line. A sentence 
before the `for` loop describing the overall approach might be useful though.



Comment at: clang/lib/Serialization/ASTWriter.cpp:4550-4554
+if (isModuleMap(File.getFileCharacteristic()) &&
+!isSystem(File.getFileCharacteristic()) &&
+!AffectingModuleMaps.empty() &&
+AffectingModuleMaps.find(Cache->OrigEntry) ==
+AffectingModuleMaps.end()) {

You can reduce nesting by inverting this condition and `continue`.



Comment at: clang/lib/Serialization/ASTWriter.cpp:4562-4563
+  NonAffectingFileIDs.back().ID == FID.ID - 1) {
+// If the previous file was non-affecting as well, just extend its 
entry
+// with our information.
+NonAffectingFileIDs.back() = FID;

I'd slightly prefer the comment *before* the `if`, due to how folding tends to 
work in editors (see the comment even when the code is folded). Probably relies 
on dropping the `else` (see below).



Comment at: clang/lib/Serialization/ASTWriter.cpp:4568
+NonAffectingFileIDsAdjustments.back() = Count;
+  } else {
+NonAffectingFileIDs.push_back(FID);

I suggest `continue` before the `else` to avoid adding nesting for insertion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D136624#3900051 , @jansvoboda11 
wrote:

>> - Is there a change in cycles/instructions when the module cache is hot? 
>> (presumably the common case)
>
> I didn't notice this (but didn't look for it specifically). How could that 
> affect performance for PCM writes?

It wouldn't check write perf, but it's part of the overall build perf impact. 
This being neutral (no regression) could help to justify landing the change 
even when there's a penalty for a cold modules cache. My interest in 
(implicitly-discovered) explicitly-built modules was similar (if that's 
neutral, then a regression here is less critical).

Partly, trying to dig into why read speeds got slower. But maybe that was noise 
that went away though when you switched to cycles/instructions?

> Loaded locations make up 68% of all calls to `getAdjustment()`, so it's good 
> it's the first thing we check for. Around 4% of all calls are for locations 
> before the first non-affecting module. That's the second special case. Around 
> 28% are pointing after the last non-affecting module, and the current 
> revision has a special case for that as well. I think it would make sense to 
> prioritize this check. Only the remaining 0.3% of calls do the actual binary 
> search.

Great; looking forward to seeing new numbers.

(BTW, if you check before/after last non-affecting module, does one of those 
subsume the is-loaded check entirely? Looking at `isLoadedOffset()` makes me 
think it might. If so, maybe you can replace that check with an assertion.)

In D136624#3900051 , @jansvoboda11 
wrote:

>> - Are the PCMs bit-for-bit identical now when a non-affecting module is 
>> added to the input? (If not, why not?)
>
> They are not in the current revision, but I'll create a patch that makes them 
> so (we also need to adjust the `NumCreatedFileIDs`).

Great. Seems like another motivation to land this change (despite a 
regression), as it can impact meta-build perf if/when artifacts are being 
archived/shared in a larger context. Specifically, if the PCM artifacts are 
more stable, then:

- They take up less aggregate space in CAS storage.
- Later build steps get more cache hits.

Overall this patch seems like a good step to me; I don't think 1-2% for a 
single compilation on a cold cache is that bad, assuming a hot cache doesn't 
regress:

- Across a single build, the cache gets hot pretty quickly as most compilations 
reuse the same modules.
- Across incremental builds, most of the cache (at least system modules) will 
(usually) be hot.
- For explicit builds, it sounds like there's no regression anyway.




Comment at: clang/include/clang/Basic/SourceManager.h:1831-1833
+  bool isLoadedOffset(SourceLocation::UIntTy SLocOffset) const {
+return SLocOffset >= CurrentLoadedOffset;
+  }

The logic for `isLoadedOffset()` suggests that it could maybe be subsumed with 
"location past the end"?



Comment at: clang/test/Modules/non-affecting-module-maps-source-locations.m:32
+
+// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o

jansvoboda11 wrote:
> dexonsmith wrote:
> > dexonsmith wrote:
> > > This is exercising the code, but it could do one better and check if the 
> > > output PCMs are bit-for-bit identical when we (now) expect them to be.
> > > 
> > > Maybe you could do this by having two run lines: one that includes `-I 
> > > %t/second` and another that doesn't. Then check if the output PCMs are 
> > > equal.
> > (Or if the PCM isn't bit-for-bit identical yet, maybe at least the AST 
> > block should be...)
> Yes, I'll probably drop this test entirely and just check the PCM files are 
> bit-for-bit identical when a non-affecting file is not loaded at all.
That sounds great.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D135220#3898849 , @hans wrote:

> Relatedly, we ran into a problem with `clang-cl /showIncludes` not including 
> all files in its output when they're linked: 
> https://github.com/llvm/llvm-project/issues/58726

Interestingly, that discussion points out that `-MD` works correctly:

  $ clang -MD -c /tmp/a.cc && cat a.d 
  a.o: /tmp/a.cc /tmp/foo.h /tmp/bar.h

I presume this is because we've pushed FileEntryRef through dependency 
tracking. Just need to push it through more places, I think.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135220/new/

https://reviews.llvm.org/D135220

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D135220#3870991 , @hans wrote:

> One thought, which I'm not sure is relevant, is that this is only observable 
> for headers which are included more than once, which is rare because normally 
> there are include guards (or pragma once). `isFileMultipleIncludeGuarded` 
> isn't tracked by the FileManager, but otherwise maybe that could have been a 
> useful heuristic: don't merge header files without include guards?

Maybe that heuristic would work a level up, in SourceManager.

- If multiple-include-guarded, create one FileID per FileEntry.
- Else, create one FileID per FileEntryRef.

Although it gets complicated with language features like `#import` in 
Objective-C, where textual inclusion is implicitly multiple-include-guarded. 
Consider a file included both using `#include` (not guarded) and as `#import` 
(guarded).

And I'm not sure we really want to split the FileIDs... that seems like a 
potential performance regression. Instead, I think we just want to track (at 
the use site) how the file was referenced.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135220/new/

https://reviews.llvm.org/D135220

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D136624#3893001 , @jansvoboda11 
wrote:

> I tried implementing your suggestion (merging ranges of adjacent 
> non-affecting files and avoiding `FileID` lookups), but the numbers from 
> `-ftime-trace` are very noisy. I got more stable data by measuring clock 
> cycles and instruction counts, but nothing conclusive yet.
>
> Compilation of `CompilerInvocation.cpp` with implicit modules.
>
> - previous approach with vector + `FileID` lookup: +0.64% cycles and +1.68% 
> instructions,
> - current approach with merged `SourceRange`s: +0.38% cycles and +1.11% 
> instructions.
>
> I'll post here as I experiment more and get more data.

Nice; that seems like a bit of an improvement.

I'm curious; are system modules allowed to be non-affecting yet, or are they 
still assumed to be affecting? (It's the system modules that I think are most 
likely to be adjacent.)

My intuition is that there is likely some peephole that would be quite 
effective, that might not be useful for general `getFileID()` lookups.

- I already suggested "same as last lookup?"... I'm curious if that'll help. 
Maybe that's already in `getFileID()`, but now that you've factored out that 
call, it could be useful to replicate.
- You could also try: "past the the last non-affecting module?"
- You could also try: "before the first non-affecting module?"

I suspect you could collect some data to guide this, such as, for loaded 
locations (you could ignore "local" locations since they already have a 
peephole):

- Histogram of "loaded" vs. "between" vs. "after" non-affecting modules.
- Histogram of "same as last" vs. "same as last-1" vs. "different from last 2".
- [...]

Other things that might be useful to know:

- What effect is the merging having (or would it have)? (i.e., what's the 
histogram of "adjacent" non-affecting files? (e.g.: 9 ranges of non-affecting 
files, with two blocks of 5 files and seven blocks of 1 (which aren't adjacent 
to any others)))
- Is there a change in cycles/instructions when the module cache is hot? 
(presumably the common case)
- Are the PCM artifacts smaller?
- Are the PCMs bit-for-bit identical now when a non-affecting module is added 
to the input? (If not, why not?)
- What's the data for implicitly-discovered, explicitly-built modules?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Modules/non-affecting-module-maps-source-locations.m:32
+
+// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o

dexonsmith wrote:
> This is exercising the code, but it could do one better and check if the 
> output PCMs are bit-for-bit identical when we (now) expect them to be.
> 
> Maybe you could do this by having two run lines: one that includes `-I 
> %t/second` and another that doesn't. Then check if the output PCMs are equal.
(Or if the PCM isn't bit-for-bit identical yet, maybe at least the AST block 
should be...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D136624#3888387 , @jansvoboda11 
wrote:

> I tried optimizing this patch a bit. Instead of creating compact data 
> structure and using binary search to find the preceding non-affecting file, I 
> now store the adjustment information for each `FileID` in a vector. During 
> deserialization, `FileID` is simply used as an index into `SLocEntryInfos`. 
> That didn't yield any measurable improvement in performance, though. I think 
> the regression must be coming from the `SourceLocation`/`Offset` to `FileID` 
> translation.
>
> I don't see any obvious way to work around that. 
> `SourceManager::getFileIDLocal()` already implements some optimizations that 
> makes accessing nearby offsets fast. A separate `SourceManager` would avoid 
> this bottleneck, but I'm not sure how much work that would entail (seems 
> substantial).
>
> @Bigcheese LMK if you're fine with the performance implications here.

I don't think you need to call `getFileID()` inside `getAdjustment()`. There is 
also an opportunity for a peephole, if `getAdjustment()` remains expensive.

I've left some comments on the previous version of the patch since it's not 
obvious to me how to avoid the `getFileID()` call in the new version.

In D136624#3888097 , @jansvoboda11 
wrote:

> The serialization slowdown I understand, but I expected deserialization to 
> get faster, since we now have less of `SourceManager` to look through.

Seems worth digging into the deserialization regression. Does the PCM actually 
get smaller and the ranges more condensed?

One quick test would be to manufacture a situation where two output PCMs would 
previously have different non-affecting inputs, but now should be bit-for-bit 
identical. Are they, in fact, bit-for-bit identical? If not, maybe there's 
something funny to look into...




Comment at: clang/include/clang/Serialization/ASTWriter.h:449-452
+  /// Exclusive prefix sum of the lengths of preceding non-affecting inputs.
+  std::vector NonAffectingInputOffsetAdjustments;
+  /// Exclusive prefix sum of the count of preceding non-affecting inputs.
+  std::vector NonAffectingInputFileIDAdjustments;

Can you collect a histogram for how big these vectors are? Can we avoid pointer 
chasing in the common case by making them `SmallVector` of some size during 
lookup?




Comment at: clang/lib/Serialization/ASTWriter.cpp:2054
 // Starting offset of this entry within this module, so skip the dummy.
-Record.push_back(SLoc->getOffset() - 2);
+Record.push_back(getAdjustedOffset(SLoc->getOffset()) - 2);
 if (SLoc->isFile()) {

Can we shift this `getAdjustedOffset()` computation to after deciding whether 
to skip the record?



Comment at: clang/lib/Serialization/ASTWriter.cpp:5282-5283
+
+SourceLocation::UIntTy
+ASTWriter::getAdjustment(SourceLocation::UIntTy Offset) const {
+  if (PP->getSourceManager().isLoadedOffset(Offset) ||

How often does `getAdjustment()` return the same answer in consecutive calls? 
If at all common, this would likely benefit from a peephole:
```
lang=c++
Optional ASTWriter::CachedAdjustmentRange;
Optional ASTWriter::CachedAdjustment;

SourceLocation::UIntTy
ASTWriter::getAdjustment(SourceLocation::UIntTy Offset) const {
  // Check for 0.
  //
  // How fast is "isLoadedOffset()"? Can/should we add a peephole, or is it 
just bit
  // manipulation? (I seem to remember it checking the high bit or something, 
but if
  // it's doing some sort of look up, maybe it should be in the slow path so it 
can
  // get cached by LastAdjustment.)
  if (PP->getSourceManager().isLoadedOffset(Offset) ||
  NonAffectingInputs.empty())
return 0;

  // Check CachedAdjustment.
  if (CachedAdjustment && CachedAdjustmentRange->includes(Offset))
return *CachedAdjustment;

  // Call getAdjustmentSlow, which updates CachedAdjustment and 
CachedAdjustmentRange.
  // It's out-of-line so that `getAdjustment` can easily be inlined without 
inlining
  // the slow path.
  //
  // LastAdjustmentRange would be the size of the "gap" between this adjustment
  // level and the next one (end would be UINTMAX if it's after the last
  // non-affecting range).
  return getAdjustmentSlow(Offset);
}
```




Comment at: clang/lib/Serialization/ASTWriter.cpp:5289-5290
+? NonAffectingInputs.end()
+: llvm::lower_bound(NonAffectingInputs,
+PP->getSourceManager().getFileID(Offset));
+  unsigned Idx = std::distance(NonAffectingInputs.begin(), It);

Why do you need to call `getFileID()` here?

Instead, I would expect this to be a search through a range of offsets (e.g., 
see my suggestion at https://reviews.llvm.org/D106876#3869247 -- `DroppedMMs` 
contains SourceLocations, not FileIDs).

Two benefits:
1. You don't need 

[PATCH] D106876: Remove non-affecting module maps from PCM files.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D106876#3869447 , @jansvoboda11 
wrote:

> I agree that avoiding serializing non-affecting input files is the better 
> approach. Your code is more or less what I had in mind, thanks for sketching 
> it out!
> The number of ignored module maps is typically around 70 - 110 on macOS (when 
> you allow system module maps to be treated as non-affecting), and those are 
> at the start of the `SourceManager` block. I might implement this approach 
> and test out if there's a noticeable performance impact. Maybe we could use 
> something similar to the optimization in `SourceManager::getFileIDLoaded()`.

Interesting. In that case it'd probably be profitable to do a pass after 
sorting to merge adjacent SourceLoc ranges. If there are many ignored system 
modules they may well be immediately adjacent and they'd resolve to a single 
range.

> I remember having a separate `SourceManager` came up before, when 
> investigating other issues. Though I think you want to merge `SourceManager`s 
> earlier than on serialization. I think other data structures might hold a 
> `SourceLocation` pointing into the module-map-specific `SourceManager`. How 
> can you tell which `SourceManager` it came from?

I'm curious whether this was a problem before, when there were two 
SourceManagers (before they were combined into one). It's possible that it's 
just obvious from context which `SourceManager` to use; that there isn't 
ambiguity in practice (because of where the SourceLocs are stored, or the 
source language, or something?).

Note that fully splitting the source manager might be worth doing (eventually) 
as a follow up, even if there aren't performance problems with the remapping 
approach. (Unless this patch-and-the-bug-fixes will address all the performance 
problems? I don't think it totally solves the quadratic use of SourceLocation 
bit space by module maps, but maybe it does?)

> This could be prevented by merging the module-map-specific `SourceManager` 
> into the main one when returning non-null result from `Module 
> *ModuleMap::lookupModule(StringRef Name)`, were that the only way to get a 
> module.

Ah, yeah, on-demand, but sooner than I was thinking. SGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106876/new/

https://reviews.llvm.org/D106876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106876: Remove non-affecting module maps from PCM files.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D106876#3869247 , @dexonsmith 
wrote:

> A further (more involved) approach would be to separate module maps into a 
> separate SourceManager, so that their source locations don't affect other 
> input files. Then only module map source locations would need to be 
> translated during serialization. (Now that FileManager has the capability to 
> remap file contents, I think the commit that merged the SourceManagers could 
> be effectively reverted.)

Throwing another idea out there... As a sort of compromise to avoid changing 
serialization/deserialization too much, maybe a hybrid could make sense:

- During compilation, keep newly-found/discovered module maps in a separate 
SourceManager.
- Translate all serialized module map locations to the main SourceManager 
during serialization. Could do this up front (collected non-ignored module 
maps) or maybe even on-demand (need to translate this location so add it to the 
main source manager).

(Or something along these lines...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106876/new/

https://reviews.llvm.org/D106876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106876: Remove non-affecting module maps from PCM files.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

As an end goal, it seems strictly better for build systems / distribution / 
artifact storage / caching if the output PCM has been canonicalized as-if the 
module maps weren't found, rather than tracking both which module maps were 
discovered and which to ignore. Ideally, we'd find a way to canonicalize the 
output efficiently and correctly... along the lines of this patch, but with a 
bug fix somewhere.

It may not be too hard/expensive to translate the source locations. E.g., you 
could build up a vector:

  SmallVector DroppedMMs;
  
  // fill up DroppedMMs with MMs...
  ...
  
  // sort to create map.
  sort(DroppedMMs);
  
  // accumulate offsets.
  SmallVector AccumulatedOffset;
  uint64_t Total = 0;
  AccumulatedOffset.reserve(DroppedMMs.size() + 1);
  AccumulatedOffset.push_back(0);
  for (MM : DroppedMMs)
AccumulatedOffset.push_back(Total += MM.size());
  
  // later, translate during serialization
  Error serializeSourceLoc(SourceLoc SL) {
if (DroppedMMs.empty())
  return SerializeSourceLocImpl(SL);
auto I = llvm::lower_bound(DroppedMMs, SL);
assert((I == MMs.end() || !I->contains(SL)) &&
   "Serializing a location from an ignored module map???");
return serializeSourceLocImpl(SL - AccumulatedOffset[I - MMs.begin()]);
  }

Then a `std::lower_bound` into `DroppedMMs` tells you how much to adjust any 
given SourceLoc by. Serializing a source location would go through this map. 
Presumably, the number of dropped files will be relatively small (number of 
ignored module maps) so the binary search should be fast. Probably there would 
be good peepholes for common cases (such as tracking `LastDroppedMM` to avoid 
repeating the same search).

A further (more involved) approach would be to separate module maps into a 
separate SourceManager, so that their source locations don't affect other input 
files. Then only module map source locations would need to be translated during 
serialization. (Now that FileManager has the capability to remap file contents, 
I think the commit that merged the SourceManagers could be effectively 
reverted.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106876/new/

https://reviews.llvm.org/D106876

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D135220#3862058 , @hans wrote:

> The build system folks replied saying they're not using symlinks, but hard 
> links for compiler inputs. Dropping the `-s` flag in the repro above 
> demonstrates this.

I think this only happened to work before. If `/tmp/a.cc` changes to these file 
contents:

  int foo_table[] = {
#include "/tmp/foo.h"
  };
  int bar_table[] = {
#include "/tmp/bar.h"
  };
  int foo2_table[] = {
#include "/tmp/foo.h"
  };

then I'm getting the following with my system clang:

  # 1 "/tmp/a.cc"
  # 1 "" 1
  # 1 "" 3
  # 414 "" 3
  # 1 "" 1
  # 1 "" 2
  # 1 "/tmp/a.cc" 2
   int foo_table[] = {
  # 1 "/tmp/foo.h" 1
  1, 2, 3
  # 3 "/tmp/a.cc" 2
   };
   int bar_table[] = {
  # 1 "/tmp/bar.h" 1
  1, 2, 3
  # 6 "/tmp/a.cc" 2
   };
   int foo2_table[] = {
  # 1 "/tmp/bar.h" 1
  1, 2, 3
  # 9 "/tmp/a.cc" 2
   };

Note that `foo2_table` now points at `bar.h` but should point at `foo.h`. I 
presume after the present patch it'll point at `foo.h` (although I admit I 
didn't test).

The root cause is the same, and matches @benlangmuir's analysis in response to 
the testcase failure on Windows:

- The FileManager is merging files based on their observed inode (regardless of 
symlink or hardlink).
- Previously, filenames were reported based on the most recent access path: 
"last-ref".
- Now, they're reported based on the original access path: "first-ref".
- In neither case is it sound.

Here are a few solutions I can see:

1. Change FileManager to only merge files with matching "realpath", seeing 
through symlinks but not hardlinks (could use observed inode collisions as a 
hint to do an `lstat` to avoid running an `lstat` every time). Then 
SourceManager will give a different FileID to these files.
2. Change SourceManager to have different FileID to these files, even though 
they were merged by FileManager.
3. Change the `#include` stack to track the accessed filename (the 
`FileEntryRef`) in addition to the SLoc, even though they have the same FileID.
4. Require clients to keep contents distinct if they it matters (the 
(surprising) status quo).

What's in tree (and has been for a long time) is (4). (1) seems like the right 
solution to me.

As a heuristic on top of (4), "last-ref" (before this patch) probably gets the 
answer the user expects more often than "first-ref" (this patch) does, which I 
assume is why it was originally implemented. However, it's unpredictable and 
requires mutating a bunch of state where mutations are hard to reason about.

IMO, "first-ref" is easier to make sound, since it's an immutable property, so 
this patch is an incremental improvement; it makes the fuzzy modeling easier to 
observe but also perhaps more predictable. If we care about fixing hardlink 
include names ("correct-ref") we should fix them in all cases (ideally with 
(1)), but I wonder how urgent it is.

@hans, WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135220/new/

https://reviews.llvm.org/D135220

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I would think we could convert every assert(0) to either 
`llvm::report_fatal_error` (guaranteed trap) or `llvm_unreachable()` (trap or 
optimize, depending on CMAKE configuration). The C API usage checks seem like 
good candidates for the former.

Also, not sure if everyone noticed, but the latter can now be configured to 
always trap by turning off the “optimize” CMAKE flag. This seems useful for 
fuzzing situations where you may not want asserts builds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135551/new/

https://reviews.llvm.org/D135551

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D135220#3840257 , @goncharov wrote:

> It's possbile to make it deterministic by making headers unique though.

Also, this is the first time you've mentioned non-determinism. Is this 
non-deterministic?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135220/new/

https://reviews.llvm.org/D135220

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D135220#3840257 , @goncharov wrote:

> In D135220#3840177 , @dexonsmith 
> wrote:
>
>> In D135220#3839671 , @goncharov 
>> wrote:
>>
>>> That change might be problematic for content addressing storages. E.g. 
>>> clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as 
>>> clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. 
>>> cc @dexonsmith
>>
>> FWIW, I agree with Ben that this change seems like it should improve 
>> consistency for symlinked content. Knowing the failure mode / having 
>> reproduction steps would be helpful to track down your corner case!
>>
>> (There are many subtle interactions around this stuff in clang so it’s hard 
>> to make forward progress.)
>
> What I see in this  clang/test/Driver/cl-pch-showincludes.cpp is that e.g. 
> for run on :25 it now emits
>
> Note: including file: ...header2.h
> Note: including file:  ...header1.h
> Note: including file: ...header1.h
>
> instead of
>
> Note: including file: ...header2.h
> Note: including file:  ...header1.h
> Note: including file: ...header3.h
>
> as header3 -> header1. It's possbile to make it deterministic by making 
> headers unique though.

And can you share environment details?

- What OS are you on?
- Which test files are linked to which?
- Are these symbolic links or hard links?
- (etc.)

Maybe you can even share steps to reproduce your environment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135220/new/

https://reviews.llvm.org/D135220

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D135220#3839671 , @goncharov wrote:

> That change might be problematic for content addressing storages. E.g. 
> clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as 
> clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. 
> cc @dexonsmith

FWIW, I agree with Ben that this change seems like it should improve 
consistency for symlinked content. Knowing the failure mode / having 
reproduction steps would be helpful to track down your corner case!

(There are many subtle interactions around this stuff in clang so it’s hard to 
make forward progress.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135220/new/

https://reviews.llvm.org/D135220

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

SGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134456/new/

https://reviews.llvm.org/D134456

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D134456#3827478 , @aaron.ballman 
wrote:

> In D134456#3827410 , @dexonsmith 
> wrote:
>
>> In D134456#3827377 , 
>> @aaron.ballman wrote:
>>
>>> Another thought I had is: when PGO is enabled, can we diagnose when PGO 
>>> countermands an explicit annotation? Something to at least alert the user 
>>> "hey, look over here, this suggestion was wrong" so they have more of a 
>>> chance of knowing something is up?
>>
>> Seems pretty noisy, much like when the inliner countermands an `inline` 
>> hint. Maybe an optimization remark would be appropriate, allowing advanced 
>> users to selectively enable them in a critical section when debugging a 
>> performance issue?
>
> Hmmm, on the one hand, any optimization hint is really an advanced user 
> feature and so the opt remark might make sense. On the other hand, when I 
> posted about `[[likely]]` and PGO on Twitter (not exactly what I'd call a 
> scientific poll, take with an ocean of salt!), multiple people were surprised 
> and expected PGO to honor the attribute, which suggests users may want this 
> information. I don't have a good feel for what the user expectations are 
> here, so I think an opt remark would be the most conservative way to start, 
> and if we find that it's too hidden and non-expert users really need 
> warnings, we can add a warning at that point. WDYT?

Let's say it *was* scientific: even then, I disagree with your assertion that 
users *really* want this information, just because they were surprised about 
what happens. Many users will be surprised when told that the optimizer 
reorders their statements; or, more relevantly, that adding `inline` doesn't 
cause a function to be inlined; it doesn't follow that we should diagnose those 
situations.

I think what users want is good performance, and a way to fix the performance 
when it's poor. If optimization remarks aren't serving the purpose of the 
latter (which I believe was their design goal), maybe they can be improved 
somehow.

Optimization annotations are generally annoying for users since they give the 
illusion of control, and users feel frustrated when they aren't followed. 
Adding a diagnostic sounds nice, since then "at least they have some 
visibility"... but in practice the optimizer supersedes user expectations 
*everywhere*; you'll find it challenging to enable such diagnostics without 
getting back a wave of non-actionable diagnostics. IMO, `[[likely]]` isn't 
special here.

Another point: having a diagnostic fire (failing a `-Werror` build) depending 
on the content of the profile passed to `-fprofile-use` seems pretty hostile to 
build workflows.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134456/new/

https://reviews.llvm.org/D134456

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D134456#3827377 , @aaron.ballman 
wrote:

> In D134456#3827332 , @dexonsmith 
> wrote:
>
>> In D134456#3827263 , 
>> @aaron.ballman wrote:
>>
>>> My worry is that parallel attributes will be used as:
>>>
>>>   #if __has_cpp_attribute(clang::likely_but_honor_this_one)
>>>   #define LIKELY [[clang::likely_but_honor_this_one]]
>>>   #elif __has_cpp_attribute(likely)
>>>   #define LIKELY [[likely]]
>>>   #else
>>>   #define LIKELY
>>>   #endif
>>
>> To be clear, I was imagining:
>>
>>   if (always_false()) [[likely]] [[clang::nopgo]] {
>> // ...
>>   }
>>
>> where ``nopgo`` might be independently useful for telling clang to ignore 
>> any collected PGO data when estimating branch weights in a particular 
>> control flow block.
>>
>> Some users might combine the two into a macro ("always ignore the profile 
>> when I say something is likely!"), but I don't think there'd be a cascade.
>
> Ah, I see, thank you for clarifying! Does that seem like a generally useful 
> attribute to you? (It seems like it could be, but this isn't my area of 
> expertise.)

Well, "generally useful" might be a stretch. Probably most users wouldn't 
want/need this. But if a user wants fine-grained control of the optimizer 
(probably not achievable, really), then turning off PGO seems like a knob they 
might want.

On the other hand, maybe `[[nopgo]]` is all that the safety-critical code 
should be using. IIRC, `[[likely]]` is really saying "hint: the other path is 
cold", and the optimizer pessimists the other path. Perhaps the safety-critical 
crowd just wants to turn off all pessimization; if so, they'll find that adding 
`[[likely]]` to the must-fail-quickly error path will do bad things to the 
non-error path.

> Another thought I had is: when PGO is enabled, can we diagnose when PGO 
> countermands an explicit annotation? Something to at least alert the user 
> "hey, look over here, this suggestion was wrong" so they have more of a 
> chance of knowing something is up?

Seems pretty noisy, much like when the inliner countermands an `inline` hint. 
Maybe an optimization remark would be appropriate, allowing advanced users to 
selectively enable them in a critical section when debugging a performance 
issue?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134456/new/

https://reviews.llvm.org/D134456

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D134456#3827332 , @dexonsmith 
wrote:

> In D134456#3827263 , @aaron.ballman 
> wrote:
>
>> My worry is that parallel attributes will be used as:
>>
>>   #if __has_cpp_attribute(clang::likely_but_honor_this_one)
>>   #define LIKELY [[clang::likely_but_honor_this_one]]
>>   #elif __has_cpp_attribute(likely)
>>   #define LIKELY [[likely]]
>>   #else
>>   #define LIKELY
>>   #endif
>
> To be clear, I was imagining:
>
>   if (always_false()) [[likely]] [[clang::nopgo]] {
> // ...
>   }
>
> where ``nopgo`` might be independently useful for telling clang to ignore any 
> collected PGO data when estimating branch weights in a particular control 
> flow block.
>
> Some users might combine the two into a macro ("always ignore the profile 
> when I say something is likely!"), but I don't think there'd be a cascade.

(In practice a use of `nopgo` probably needs to drop the profile for the whole 
function, but maybe that's fine...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134456/new/

https://reviews.llvm.org/D134456

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D134456#3827263 , @aaron.ballman 
wrote:

> My worry is that parallel attributes will be used as:
>
>   #if __has_cpp_attribute(clang::likely_but_honor_this_one)
>   #define LIKELY [[clang::likely_but_honor_this_one]]
>   #elif __has_cpp_attribute(likely)
>   #define LIKELY [[likely]]
>   #else
>   #define LIKELY
>   #endif

To be clear, I was imagining:

  if (always_false()) [[likely]] [[clang::nopgo]] {
// ...
  }

where ``nopgo`` might be independently useful for telling clang to ignore any 
collected PGO data when estimating branch weights in a particular control flow 
block.

Some users might combine the two into a macro ("always ignore the profile when 
I say something is likely!"), but I don't think there'd be a cascade.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134456/new/

https://reviews.llvm.org/D134456

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D134456#3827263 , @aaron.ballman 
wrote:

> In D134456#3827185 , @dexonsmith 
> wrote:
>
>> This safety scenario sounds like it could differ within a file. Is a flag 
>> really the right control? Maybe what you want is a `[[noprofile]]`, similar 
>> to `[[noopt]]`, which selectively disables the profile where the user wants 
>> fine-grained control to ignore the profile.
>
> My understanding is that it's pretty rare for safety critical code to mix 
> with non-safety critical code, so a flag sounds like the correct granularity 
> to me in terms of the use cases I'm aware of.

In that case, maybe those files should just turn off PGO entirely. There's 
already a command-line for that:

- `-fprofile-use=...`: compiler should use the profile to improve performance 
(user hints lose when there's disagreement)
- default: no access to a profile; user hints by default

Maybe (if it doesn't exist yet) `-fno-profile-use` would be useful for easy 
cancellation.

In safety-critical code where you don't entirely trust the 
profile/coverage/compiler to do the right thing, why risk having the feature 
enabled at all?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134456/new/

https://reviews.llvm.org/D134456

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D134456#3827040 , @aaron.ballman 
wrote:

> In D134456#3819185 , @rnk wrote:
>
>> In D134456#3819042 , 
>> @aaron.ballman wrote:
>>
>>> Alternatively, we could document that we don't follow the intent of the 
>>> standard feature and that's just the way things are, and add a command line 
>>> option to honor that intent. However, given that GCC honors the attribute 
>>> over PGO, I think we should change our default behavior.
>>
>> That makes sense to me. I think it's reasonable to request folks to add a 
>> flag in the context of a bug fix, but it would be too much to ask a drive by 
>> contributor to go through the multi-step process to change the default flag 
>> behavior.
>
> I guess I view it differently -- I think users should get the correct 
> behavior by default rather than have to opt into it, and I'm not certain that 
> "it's a drive-by contribution" is a good driver of user experience decisions. 
> Either way, I think we need an explicit decision as to which approach we 
> think is *right* and go from there in terms of how to achieve that goal. 
> (It'd be a bit different if I thought this patch was incremental progress, 
> but from my current views on the topic, I actually think the patch is a 
> further regression away from where we want to be.)
>
> I'm adding in @dexonsmith as "branch weights" code owner and @dnovillo as 
> "SampleProfile and related parts of ProfileData" code owner on the LLVM side 
> to see if they have opinions on default behaviors (if there are other PGO 
> experts to bring in, please sign them up).

The design of PGO was to use user hints when there’s no coverage, but basically 
ignore them if theres enough data to say otherwise.

This safety scenario sounds like it could differ within a file. Is a flag 
really the right control? Maybe what you want is a `[[noprofile]]`, similar to 
`[[noopt]]`, which selectively disables the profile where the user wants 
fine-grained control to ignore the profile.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134456/new/

https://reviews.llvm.org/D134456

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95502: WIP: Frontend: Adopt llvm::vfs::OutputBackend in CompilerInstance

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith abandoned this revision.
dexonsmith added a comment.
Herald added a project: All.

This is superseded by https://reviews.llvm.org/D133509.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95502/new/

https://reviews.llvm.org/D95502

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a subscriber: arphaman.
dexonsmith added inline comments.



Comment at: clang/test/FixIt/format.m:195-208
-  NSLog(@"%C", 0x260300);  // expected-warning{{format specifies type 
'unichar' (aka 'unsigned short') but the argument has type 'int'}}
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unsigned short)"
 
   typedef unsigned short unichar;
-  
-  NSLog(@"%C", 0x260300);  // expected-warning{{format specifies type 
'unichar' (aka 'unsigned short') but the argument has type 'int'}}

aaron.ballman wrote:
> Hmmm, did we intend to change the behavior here? `%C` is an extension, so 
> it's not clear to me if we wanted to modify this behavior or not. CC 
> @rjmccall and @dexonsmith for some ObjC opinions.
I’m not sure, but I agree it’s suspicious. Besides John, Mike Ash might be 
another person to check with (`@`-completion isn’t working for me on my phone 
right now, but perhaps someone else will have more luck?). Or, @arphaman, any 
other suggestions?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132568/new/

https://reviews.llvm.org/D132568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D130303#3724247 , @rnk wrote:

> Pinging alternative reviewer +@dexonsmith for a libclang API addition

Looks reasonable to me -- this only changes behaviour of the existing API when 
there was corruption before -- but if the goal is to get a vendor of 
libclang-as-a-stable-API to sign off, I can't help.

@arphaman, if you're busy, is there someone else that could take a quick look?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: llvm/include/llvm/ADT/SCCIterator.h:165-170
+  SCCProxy operator*() const {
 assert(!CurrentSCC.empty() && "Dereferencing END SCC iterator!");
 return CurrentSCC;
   }
 
+  SCCProxy operator->() const { return operator*(); }

dblaikie wrote:
> rusyaev-roman wrote:
> > dblaikie wrote:
> > > I always forget in which cases you're allowed to return a proxy object 
> > > from an iterator - I thought some iterator concepts (maybe random access 
> > > is the level at which this kicks in?) that required something that 
> > > amounts to "there has to be a real object that outlives the iterator"
> > > 
> > > Could you refresh my memory on that/on why proxy objects are acceptable 
> > > for this iterator type? (where/how does this iterator declare what 
> > > concept it models anyway, since this removed the facade helper?)
> > A proxy object is allowed to be returned while dereferencing an `input 
> > iterator` (https://en.cppreference.com/w/cpp/named_req/InputIterator#Notes)
> > 
> > ```
> > The reference type for an input iterator that is not also a 
> > LegacyForwardIterator does not have to be a reference type: dereferencing 
> > an input iterator may return a proxy object or value_type itself by value
> > ```
> > 
> > For our case (that's `forward iterator`) we need to satisfy the following 
> > thing:
> > ```
> >  The type std::iterator_traits::reference must be exactly 
> >...
> >* const T& otherwise (It is constant), 
> > 
> > (where T is the type denoted by std::iterator_traits::value_type) 
> > ```
> > I'll also update the patch according to this point. Other things are ok for 
> > using a proxy object.
> Thanks for doing the legwork/quotations there.
> 
> so what's the solution here, if we're going to meet the forward iterator 
> requirements but want a proxy object?
IIRC, one solution is to store a proxy object inside the iterator, make `using 
value_type = ProxyT`, update what the stored proxy points at on `operator*()`, 
and return `ProxyT&` when dereferencing. But maybe I'm misremembering. (I'm 
pretty sure `iterator_facade_base` has machinery to help with this stuff, 
either way; might be worth looking at other uses of it.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131448/new/

https://reviews.llvm.org/D131448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D126676#3687491 , @mstorsjo wrote:

> In D126676#3687227 , @dexonsmith 
> wrote:
>
>> There are cases where it’s safe to have mismatched defines (e.g., if a 
>> define can be proven during a cheap dependency scan not to matter for a 
>> given PCH, it’s better to canonicalize away the define and share the 
>> artifact more broadly) but if I understand correctly this patch won’t 
>> preclude compiler smarts like that.
>
> Yup. Any implementation of such logic hasn’t materialized during the 10 years 
> since todos hinting that we should implement that, but it doesn’t seem to be 
> a big practical issue either, outside of false positive matches with gcc PCH 
> directories - so I guess it’s not a high priority in practice.

Note, FWIW, that clang-scan-deps is being used to canonicalize command-lines as 
of the last year; e.g., there are patches up (maybe committed?) to remove 
unused search paths for implicitly-discovered explicit modules. Finding unused 
defines is a bit harder and less urgent (search path explosion had to be fixed 
for performance parity with on-demand implicit modules, which have an unsound 
optimization of ignoring search path differences), but there’s now a framework 
where it can be reasonably implemented if/when it’s identified as a major 
bottleneck in builds.

(Fallout from unsoundness in on-demand implicit modules is a big reason 
progress has been slow in this area.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126676/new/

https://reviews.llvm.org/D126676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I haven’t reviewed the details of the patch and won’t have time to do so (at 
least for a while), but the description of the intended (more narrow) scope 
SGTM. With the scope limited to GCC directories this should be fine.

There are cases where it’s safe to have mismatched defines (e.g., if a define 
can be proven during a cheap dependency scan not to matter for a given PCH, 
it’s better to canonicalize away the define and share the artifact more 
broadly) but if I understand correctly this patch won’t preclude compiler 
smarts like that.

If I’m right, then I think this can go in once @rnk is happy. (If I’m wrong, 
maybe better to wait for one of the people I pinged.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126676/new/

https://reviews.llvm.org/D126676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129446: [clang][driver] Find Apple default SDK path

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a subscriber: arphaman.
dexonsmith added a comment.

I’m not at Apple anymore, but this is a long-standing platform decision that is 
intentional and seems unlikely to change. Note that `/usr/bin/clang` isn’t 
really clang, but a tool called `xcrun` which adds environment variables to 
help find a default SDK before executing `/path/to/toolchain/usr/bin/clang`.

When the driver is called directly it expects the build system to provide the 
appropriate SDK. There are situations where it could cause a problem for the 
driver to hunt around tor arbitrary SDKs.

If you still want to pursue this, @arphaman might be able to help find someone 
in a position to consider the impact / policy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129446/new/

https://reviews.llvm.org/D129446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129226: [clang/mac] Make -mmacos-version-min the canonical spelling over -mmacosx-version-min

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a reviewer: akyrtzi.
dexonsmith added a subscriber: akyrtzi.
dexonsmith added a comment.

LGTM too, but I’m not at Apple these days.

@akyrtzi, can you help find someone appropriate to take a quick look since 
@arphaman seems busy?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129226/new/

https://reviews.llvm.org/D129226

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128207: [clang-doc][NFC] Fix reference invalidation assertion failure.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Is there a test that can be added so this doesn’t regress?

Also, I wonder if `assign` could/should add logic to be resilient to this.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128207/new/

https://reviews.llvm.org/D128207

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127408: [clang][driver] Introduce new -fdriver-only flag

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.

LGTM with a minor comment on the test.




Comment at: clang/test/Driver/driver-only.c:13
+
+// Check that -fdriver-only respects -v.
+//

Should there be a `-check-epmty` or similar when `-v` is not passed? (This 
tests the positive, but I don’t see a test that it’s not ALWAYS verbose.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127408/new/

https://reviews.llvm.org/D127408

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125814: Improve the strict prototype diagnostic behavior

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith resigned from this revision.
dexonsmith added a comment.

(I’ll be happy with whatever you two sort out here!)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125814/new/

https://reviews.llvm.org/D125814

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125487: [Tooling/DependencyScanning] Refactor dependency scanning to produce pre-lexed preprocessor directive tokens, instead of minimized sources

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp:175
   "#define MACRO   con  tent   ", Out));
-  EXPECT_STREQ("#define MACRO con  tent\n", Out.data());
+  EXPECT_STREQ("#define MACRO con tent\n", Out.data());
 

akyrtzi wrote:
> dexonsmith wrote:
> > Can you clarify why this is changing in this patch? It’s not obvious to me 
> > why this would start optimizing internal whitespace of macros. 
> > 
> > (I also can’t remember if it’s safe/correct. Is there a way to stringify 
> > the contents of a macro? If so, this would change the result… if not, then 
> > this seems like an improvement, but maybe better for a separate patch?)
> > 
> > (If there’s a good reason to change it here, don’t want to hold it up, but 
> > it’s not explained in the commit message so it’s not obvious to me.)
> > Can you clarify why this is changing in this patch? It’s not obvious to me 
> > why this would start optimizing internal whitespace of macros.
> 
> Minimization was doing `printToNewline()` after the macro identifier, which 
> just prints verbatim, including any amount of whitespace. OTOH the new way is 
> collecting the tokens of the directive, which ignores unnecessary whitespace.
> 
> > (I also can’t remember if it’s safe/correct. Is there a way to stringify 
> > the contents of a macro? If so, this would change the result… 
> 
> AFAIK this is correct since the macro body is getting tokenized when the 
> preprocessor sees a `#define` and the unnecessary whitespace is ignored.
> 
> > if not, then this seems like an improvement, but maybe better for a 
> > separate patch?)
> 
> To clarify, `minimizeSourceToDependencyDirectives()` is only used for testing 
> purposes, once we get the tokens of the directives then we use them directly 
> during preprocessing, so there's no place for preserving the unnecessary 
> whitespace. This change is just inherent to the difference in implementation 
> for minimization vs tokenization.
SGTM! Thanks for explaining. (I’ll let others review in detail!)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125487/new/

https://reviews.llvm.org/D125487

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125487: [Tooling/DependencyScanning] Refactor dependency scanning to produce pre-lexed preprocessor directive tokens, instead of minimized sources

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp:175
   "#define MACRO   con  tent   ", Out));
-  EXPECT_STREQ("#define MACRO con  tent\n", Out.data());
+  EXPECT_STREQ("#define MACRO con tent\n", Out.data());
 

Can you clarify why this is changing in this patch? It’s not obvious to me why 
this would start optimizing internal whitespace of macros. 

(I also can’t remember if it’s safe/correct. Is there a way to stringify the 
contents of a macro? If so, this would change the result… if not, then this 
seems like an improvement, but maybe better for a separate patch?)

(If there’s a good reason to change it here, don’t want to hold it up, but it’s 
not explained in the commit message so it’s not obvious to me.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125487/new/

https://reviews.llvm.org/D125487

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D122895#3511855 , @aaron.ballman 
wrote:

>> (It also seems unfortunate to regress the false positive rate of this 
>> diagnostic before `-fstrict-prototypes` is available.)
>
> `-fno-knr-functions` is available already today in trunk, so I'm not certain 
> I understand your concern there (or were you unaware we changed the flag name 
> perhaps?).

That's right, I missed it. Thanks for pointing it out!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122895/new/

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124974: [clang] Include clang config.h in LangStandards.cpp

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.

LGTM, thanks! One comment online below.




Comment at: clang/include/clang/Config/config.h.cmake:19
 #cmakedefine CLANG_DEFAULT_STD_C LangStandard::lang_${CLANG_DEFAULT_STD_C}
+// Always #define something so that missing the config.h #include at use sites
+// becomes a compile error.

I just noticed that the other comments in this file are C-style. Probably the 
new ones should be too. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124974/new/

https://reviews.llvm.org/D124974

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D122895#3511649 , @aaron.ballman 
wrote:

> In D122895#3511611 , @dexonsmith 
> wrote:
>
>> Sure, I'm all for adding a new warning for users that want a pedantic 
>> warning. Can it be put behind a separate flag, such as 
>> `-Wstrict-prototypes-pedantic`, which isn't triggered by 
>> `-Wstrict-prototypes`?
>>
>> Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in 
>> code.
>
> Doing that would then make `-Wstrict-prototypes` effectively a no-op (it 
> would still control `-Wdeprecated-non-prototype` I suppose?),

Is it necessary to make `-Wstrict-prototypes` weaker in order to move the newer 
more pedantic cases to a different name?

> but there were also people who enabled `-Wstrict-prototypes` because they 
> wanted the pedantic aspects of the warning in cases where it was firing, and 
> those folks would then be losing warning coverage without knowing it. For 
> example:
>
>   void f(){}
>
> In prior versions of Clang with `-Wstrict-prototypes` this would issue a 
> `this old-style function definition is not preceded by a prototype` 
> diagnostic, but would now be silenced entirely unless the user knew to turn 
> on a different flag.

Oh, I thought that would catch bugs like this:

  void longfunctionname(){}
  void longfunctionnames(int x){ /* do something with x */ }
  
  void g(void) {
longfunctionname(7); // oops, meant to call longfunctionnames().
  }

but if it's entirely pedantic (if the call to `longfunctionname(7)` would fail 
to compile) then I agree it'd be better to silence it unless someone asks for 
`-pedantic`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122895/new/

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D122895#3511611 , @dexonsmith 
wrote:

> Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in 
> code.

For example, it's important to have a warning that catches code like this:

  void f1(void (^block)());
  
  void f2(void) {
f1(^(int x) { /* do something with x */ });
  }

without triggering in cases that are pedantic.

(It also seems unfortunate to regress the false positive rate of this 
diagnostic before `-fstrict-prototypes` is available.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122895/new/

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a subscriber: arphaman.
dexonsmith added a comment.

In D122895#3511376 , @aaron.ballman 
wrote:

> In D122895#3511312 , @aaron.ballman 
> wrote:
>
>> However, I think the blocks behavior is a bug based on this: 
>> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728
>>  and I'd be more than happy to fix that, as I'm not checking that condition 
>> here: 
>> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579
>>  which seems like a pretty obvious thing to be checking for before warning 
>> about not having a strict prototype.
>
> I fixed this false positive bug in 4be105c98a9c7e083cd878ee1751e11160b97b4a 
> , so 
> blocks (and OpenCL) behavior should now be improved.

Thanks! I think that's the bigger issue of the two. @steven_wu or @arphaman 
will have more context though.

In D122895#3511312 , @aaron.ballman 
wrote:

> In D122895#3510165 , @dexonsmith 
> wrote:
>
>> For additional context to my questions above, even though open source clang 
>> hasn't been using `-Wstrict-prototypes`, Xcode has had it on-by-default in 
>> new projects since sometime in 2017, with project modernizations to turn it 
>> on for old projects.
>
> Thanks, that's very good to know! And also, thank you for raising the 
> questions here, I appreciate the discussion.
>
>> Warning on block and function definitions such as `^(){}` and `void f1() 
>> {}`, which are pedantically lacking a prototype but the compiler knows there 
>> are exactly zero parameters, would be super noisy for users.
>>
>> Unless I read the RFC too quickly, it doesn't look like it's adding any 
>> value, since these aren't going to change meaning. Is it possible to revert 
>> this part of the change?
>
> `-Wstrict-prototypes` is now a pedantic deprecation warning that fires any 
> time you form a function type which has no prototype, which was discussed in 
> the RFC 
> (https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/38?u=aaronballman):
>
>   Change -Wstrict-prototypes to diagnose functions without a prototype that 
> don’t change behavior in C2x, it remains off-by-default but is automatically 
> enabled by -pedantic as it’s still warning the user about a deprecation.
>
> However, I think the blocks behavior is a bug based on this: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728
>  and I'd be more than happy to fix that, as I'm not checking that condition 
> here: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579
>  which seems like a pretty obvious thing to be checking for before warning 
> about not having a strict prototype.
>
> But I'm pretty insistent on warning about the other case as it does use 
> functions without a prototype and we need *some* blanket warning for use of a 
> deprecated feature for folks who want to be strictly conforming. It sounds 
> like Apple may want to no longer enable `-Wstrict-prototypes` by default as 
> it's shifted to be a more pedantic warning than it was before -- would that 
> be a viable option for you (can you use project modernizations to turn it 
> back off for old projects)?

Sure, I'm all for adding a new warning for users that want a pedantic warning. 
Can it be put behind a separate flag, such as `-Wstrict-prototypes-pedantic`, 
which isn't triggered by `-Wstrict-prototypes`?

Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122895/new/

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125488: [Preprocessor] Make the special lexing for dependency scanning a first-class feature of the `Preprocessor` and `Lexer`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D125488#3510320 , @akyrtzi wrote:

> In D125488#3510297 , @dexonsmith 
> wrote:
>
>> [To be clear, my question was because I don't see this patch deleting the 
>> code path that minimizes / saves-minimized sources. Can/should we delete the 
>> "minimize sources" code path?]
>
> Oh, this is removed in the prior patch in the review stack 
> (https://reviews.llvm.org/D125487)

Seems unfortunate to have a temporary regression in the commit stack, since 
then you can't push incrementally (or bisect). Can the prior patch leave behind 
the feature in the DependencyFilesystem, and this patch delete it now that 
clang-scan-deps doesn't depend on it for performance? (Or ignore me if I'm 
still not understanding...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125488/new/

https://reviews.llvm.org/D125488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125488: [Preprocessor] Make the special lexing for dependency scanning a first-class feature of the `Preprocessor` and `Lexer`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D125488#3510265 , @akyrtzi wrote:

> In D125488#3510214 , @dexonsmith 
> wrote:
>
>> Is there code in DepFS that can/should be deleted as part of this patch, or 
>> in a follow-up, or is it still around as an option?

[To be clear, my question was because I don't see this patch deleting the code 
path that minimizes / saves-minimized sources. Can/should we delete the 
"minimize sources" code path?]

> After these changes, with DepFS we are using its multi-threading sharding 
> technique to cache file `stat`s and the source file directive scanning 
> results. We could use multi-threading sharding only to cache source file 
> directive scanning results, and get rid of `DepFS` altogether, but then I 
> don't see a good way to cache the file `stat`s as well, unless we want to try 
> to re-use the `FileManager` across depscan instances and make its `stat` 
> caching as efficient as DepFS (maybe by generalizing the multi-threading 
> sharding technique and using it in `FileManager`).
>
> There's also `FileSystemStatCache` which seems like a leftover right now, but 
> we could enhance it and have it shared by individual `FileManager` instances 
> (instead of sharing the same `FileManager` in depscan instances).
>
> What do you think?

FWIW, I don't think we should be sharing FileManager at all, since it has state 
that makes sharing it unsound.

I like the direction of trying to remove FSStatCache. I think stat/content 
caching belongs at the VFS level so DepFS seems like a better starting point 
(maybe generalized a bit). Note that DepFS isn't just amortizing `stat` cost, 
it's also avoiding reopening / `mmap`ing / `mempcy`ing files.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125488/new/

https://reviews.llvm.org/D125488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Sema/warn-strict-prototypes.m:20
+  // know it's a block when diagnosing.
+  void (^block2)(void) = ^void() { // expected-warning {{a function 
declaration without a prototype is deprecated in all versions of C}}
   };

efriedma wrote:
> dexonsmith wrote:
> > dexonsmith wrote:
> > > This is a definition, so the compiler knows that there are no parameters. 
> > > Why would we warn here? Reading 
> > > https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/18
> > >  it looks to me like an example of what @rnk was referring to, about 
> > > churning code to add `(void)` and then return back to `()` later.
> > > 
> > > (cc: @steven_wu and @rjmccall as well)
> > Specifically, `^void() { /* anything */}` is the definition of a block with 
> > zero parameters. Maybe pedantically it's lacking a prototype, but the 
> > compiler knows (since this is the definition) how many parameters there are.
> > 
> > (Same goes for `void() { /* anything */ }` at global scope; is that 
> > triggering `-Wstrict-prototypes` now too?)
> To be clear, you're asking specifically about the following two cases where 
> the behavior with -Wstrict-prototypes changed, right?  I don't think this was 
> really discussed in the RFC.
> 
> ```
> // Block
> void (^f1)(void) = ^(){};
> 
> // Function definition
> void f(void);
> void f(){}
> ```
> 
> Note that the behavior of the following did not change:
> 
> ```
> void (^f1)(void) = ^{}; // no warning
> void f2(){} // warning since clang 11.
> ```
Right, specifically those two cases that changed. Thanks for clarifying.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122895/new/

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125488: [Preprocessor] Make the special lexing for dependency scanning a first-class feature of the `Preprocessor` and `Lexer`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Awesome to see this! Looking forward to the next step (using this in normal 
preprocessing!).

> after this change, we don't minimize sources and pass them in place of the 
> real sources

Is there code in DepFS that can/should be deleted as part of this patch, or in 
a follow-up, or is it still around as an option?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125488/new/

https://reviews.llvm.org/D125488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124974: [clang] Include clang config.h in LangStandards.cpp

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D124974#3504986 , @porglezomp 
wrote:

> Ah, so it'd be a test that passes pretty trivially on any bot that doesn't 
> have a custom version of `CLANG_DEFAULT_STD_C` and `CLANG_DEFAULT_STD_CXX` 
> like the default config, but could get caught by any other bots that do set 
> it?
>
> What's the best way to get the compiler to report the standard it's using for 
> this test?
>
> For C++ I can do:
>
>   clang++ -xc++ -dM -E - < /dev/null | grep cplusplus
>
> And it will print e.g.
>
>   #define __cplusplus 199711L
>
> But that requires quite some decoding to compare against 
> `CLANG_DEFAULT_STD_CXX` which would look like `LangStandard::lang_gnucxx98`.

Yeah, that seems awkward.

My concern with lacking a test is that someone could easily remove this in the 
future when trying to prune unused `#include`s.

Another solution might be to change this to a compile error. I think it'd work 
to change the `#cmakedefine` logic to:

  #cmakedefine CLANG_DEFAULT_STD_C LangStandard::lang_${CLANG_DEFAULT_STD_C}
  // Always #define something so that missing the config.h #include at use sites
  // becomes a compile error.
  #ifndef CLANG_DEFAULT_STD_C
  #define CLANG_DEFAULT_STD_C LangStandard::lang_unknown
  #endif

and then update the code to:

  case Language::C:
if (CLANG_DEFAULT_STD_C != LangStandard::lang_unknown)
  return CLANG_DEFAULT_STD_C;
  
// The PS4 uses C99 as the default C standard.
if (T.isPS4())
  return LangStandard::lang_gnu99;
return LangStandard::lang_gnu17;

(without the `#ifdef`)

WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124974/new/

https://reviews.llvm.org/D124974

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

For additional context to my questions above, even though open source clang 
hasn't been using `-Wstrict-prototypes`, Xcode has had it on-by-default in new 
projects since sometime in 2017, with project modernizations to turn it on for 
old projects.

Warning on block and function definitions such as `^(){}` and `void f1() {}`, 
which are pedantically lacking a prototype but the compiler knows there are 
exactly zero parameters, would be super noisy for users.

Unless I read the RFC too quickly, it doesn't look like it's adding any value, 
since these aren't going to change meaning. Is it possible to revert this part 
of the change?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122895/new/

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Sema/warn-strict-prototypes.m:20
+  // know it's a block when diagnosing.
+  void (^block2)(void) = ^void() { // expected-warning {{a function 
declaration without a prototype is deprecated in all versions of C}}
   };

dexonsmith wrote:
> This is a definition, so the compiler knows that there are no parameters. Why 
> would we warn here? Reading 
> https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/18
>  it looks to me like an example of what @rnk was referring to, about churning 
> code to add `(void)` and then return back to `()` later.
> 
> (cc: @steven_wu and @rjmccall as well)
Specifically, `^void() { /* anything */}` is the definition of a block with 
zero parameters. Maybe pedantically it's lacking a prototype, but the compiler 
knows (since this is the definition) how many parameters there are.

(Same goes for `void() { /* anything */ }` at global scope; is that triggering 
`-Wstrict-prototypes` now too?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122895/new/

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added subscribers: steven_wu, rjmccall, rnk, dexonsmith.
dexonsmith added inline comments.



Comment at: clang/test/Sema/warn-strict-prototypes.m:20
+  // know it's a block when diagnosing.
+  void (^block2)(void) = ^void() { // expected-warning {{a function 
declaration without a prototype is deprecated in all versions of C}}
   };

This is a definition, so the compiler knows that there are no parameters. Why 
would we warn here? Reading 
https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/18
 it looks to me like an example of what @rnk was referring to, about churning 
code to add `(void)` and then return back to `()` later.

(cc: @steven_wu and @rjmccall as well)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122895/new/

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits