In D146426#4207118, @aaron.ballman wrote:

This feels like it's heading in the wrong direction -- the AST should not have holes in it. An invalid type should be replaced by a valid type (after diagnosing the invalid type, of course) so that we can keep as much of the AST around as possible (for example, we typically stub in int and continue compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then result in a non-null ParmVarDecl. This way, we don't need to sprinkle nullptr checks all over the compiler when inspecting a function's parameters.

Hi, thanks Vitaly!

In D145881#4204872, @Mordante wrote:

Thanks! LGTM! IIRC you have commit access right?

Changes aaron suggested. Chose zip_longest instead of enumerate.

In D109958#4206644, @nikic wrote:

FWIW, I am quite unhappy with the implementation quality of this pass, but I don't think I have the energy to deal with this. In the future, due diligence for pass enablement needs to include a review of the pass implementation by a domain expert, if this was not already done as part of the initial implementation. (Domain expert = SCEV reviewer in this context.)

This update reverts part of a previous update and removes _LIBCPP_ASAN_ANNOTATE_ONLY_LONG.

Has this patch been tested against Chromium? Has it been tested elsewhere?
The more testing it's undergone, the more confident I can be.

I still don't understand the motivation behind this change. You already explicitly set CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to ONLY for Android so why do we need to change the default?

In D144657#4206620, @ftynse wrote:

Sorry for delay, already-approved patches don't show up in the review stream :(

In D145558#4199730, @Orlando wrote:

Thank you @scott.linder for taking a look at this.

It might be worth including the rationale in the description/commit message, like you've done in other similar changes. I assume it is a performance improvement?

Yep there's an average of around 0.25% reduction in instructions retired and a 1.1% max-rss for the CTMark suite in LTO-O3-g builds (I'll update the description).

I'm leaving a bunch of edit suggestions, but feel free to ignore them! I was just going through the exercise of writing one possible implementation to prove to myself it is an actual improvement, YMMV

I appreciate the in-depth review and the suggestions all look good to me!

The diff has become quite large now so I'll could look at trying to factor out some of the suggestions into NFC patches?

In D144958#4206872, @aeubanks wrote:

a more succinct version: https://godbolt.org/z/7YTqP89Mv shows that slp-vectorizer introduces loads from poison

In D146426#4207118, @aaron.ballman wrote:

This feels like it's heading in the wrong direction -- the AST should not have holes in it. An invalid type should be replaced by a valid type (after diagnosing the invalid type, of course) so that we can keep as much of the AST around as possible (for example, we typically stub in int and continue compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then result in a non-null ParmVarDecl. This way, we don't need to sprinkle nullptr checks all over the compiler when inspecting a function's parameters.

Sorry @sebpop but I don't think it is part of ARM mapping symbols ABI standard. What type of compiler are you using when observing this? Is there any way to create test to reproduce it?

LGTM, but @phosek should also be able to confirm this is ok.

LGTM

add "<0xHEXSTRING>" to the option documentation.

Updated patch to pass clang-format check.

Rebase

I agree with @Mordante

This feels like it's heading in the wrong direction -- the AST should not have holes in it. An invalid type should be replaced by a valid type (after diagnosing the invalid type, of course) so that we can keep as much of the AST around as possible (for example, we typically stub in int and continue compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then result in a non-null ParmVarDecl. This way, we don't need to sprinkle nullptr checks all over the compiler when inspecting a function's parameters.

move vfprintf_internal and file_writer to templates

My understanding is that a more elaborate configuration scheme has been proposed in https://github.com/clangd/clangd/issues/1367, and the feedback there was (quoting Sam from this comment):

More files

I believe I understand your concerns.

Update a few more files.

Update RISCVISAInfo.* and RISCVTargetParser.*

There was objection against using /usr/modules on discourse. I assumed that libc++ owns /usr/include/c++ and hides everything under there.

Can you please upload a patch that does not reformat the entire file?
Also, add a commit message explaining what this does.

update

ping @StephenFan

can we update some tests to see the results?

Oh wait, I didn't realize you haven't landed it yet :-)

Thanks, but this does not seem to have worked: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/52610/consoleText

I just went through the patch and it LGTM. The table of contents was really useful here.

This is essentially a GPU only change. Stamping anyway.

Thanks @arsenm for taking another look.

This looks correct, although I think there are a few places where we can simplify the code to remove the explicit endianness.

I guess Android was that main reason was older API levels why don't use thread_local, probably our buildbot is still on them.
Wide use of thread_local may simplify sanitizers.

@vitalybuka could you land that revision as I don't have commiter rights?

In D146198#4205441, @pcwang-thead wrote:

In D146198#4202168, @michaelmaitland wrote:

If there are some microarchitectures that can't be modeled, just add a new subroutine to upstream if approved.

Does this mean that subtarget routines must be added to the RISCVScheduleV file since the following function needs to know about the custom subroutine to do its isa checks:

// Helper class for generating a list of resource cycles of different LMULs.
class ResourceCycles<list<ResourceCycle> resourceCycles, string mx> {

I am concerned that the RISCVScheduleV file will take on bloat due to holding subtarget related routines if this is the case.

Yes. So I posted this patch here just to discuss how we should handle this.
For example, solutions may be:

1. Add routines to RISCVScheduleV.td just as what I have done.
2. Extend TableGen to support pass functions:

// Supposes that we have a Function class to present a function object that its parameters are function parameters.
class TargetSubroutine<int base, string mx> : Function;

// Then. Supposes that we have a new bang operator to apply this function to input parameters and the result is `ret`.
class ResourceCycles<list<TargetSubroutine> subroutines, int base, string mx> {
  list<int> value = !foreach(subroutine, subroutines,
                             !apply(subroutine, base, mx)
                            );
}

// In SchedXXX.td, we can define our own routines.
class Multiplier<int base, string mx>:TargetSubroutine {
 // We return an int value calculated from mx.
 int ret = !mul(base, multiplier<mx>.value);
}

3. Some templates are flexible to specify cycles according to LMULs (I haven't figured out one...).

Ilyas Mustafazade <il.mystafa@gmail.com>
Thank you.