Page MenuHomePhabricator

lebedev.ri (Roman Lebedev)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 27 2012, 6:35 AM (342 w, 3 d)

Recent Activity

Today

lebedev.ri added inline comments to D62191: [SelectionDAG] define binops as a superset of commutative binops.
Tue, May 21, 6:44 AM · Restricted Project
lebedev.ri added inline comments to D62191: [SelectionDAG] define binops as a superset of commutative binops.
Tue, May 21, 6:24 AM · Restricted Project
lebedev.ri accepted D62191: [SelectionDAG] define binops as a superset of commutative binops.

Looks good to me.

Tue, May 21, 6:11 AM · Restricted Project
lebedev.ri added inline comments to D62115: fix a issue that clang is incompatible with gcc with -H option..
Tue, May 21, 6:11 AM · Restricted Project
lebedev.ri committed rGd8db224ecb1c: [NFC][X86][AArch64] Shift amount masking: tests that show that 'neg' doesn't… (authored by lebedev.ri).
[NFC][X86][AArch64] Shift amount masking: tests that show that 'neg' doesn't…
Tue, May 21, 6:04 AM
lebedev.ri committed rG2aee73f591da: [NFC][X86][AArch64] Add some more tests for shift amount masking (authored by lebedev.ri).
[NFC][X86][AArch64] Add some more tests for shift amount masking
Tue, May 21, 4:14 AM
lebedev.ri added inline comments to D62115: fix a issue that clang is incompatible with gcc with -H option..
Tue, May 21, 12:03 AM · Restricted Project

Yesterday

lebedev.ri added a comment to D61851: [clang-tidy] New option for misc-throw-by-value-catch-by-reference.

Looks good

Mon, May 20, 6:37 AM · Restricted Project, Restricted Project, Restricted Project
lebedev.ri added inline comments to D62115: fix a issue that clang is incompatible with gcc with -H option..
Mon, May 20, 3:05 AM · Restricted Project
lebedev.ri added a comment to D62067: [Support] Time profiler: support new PassManager.

I'm not really familiar with new-PM.
Can this perhaps have a test?

Mon, May 20, 3:01 AM · Restricted Project

Sat, May 18

lebedev.ri added a comment to D62100: [DAGCombine][X86][AMDGPU][AArch64] (srl (shl x, c1), c2) with c1 != c2 handling.

Looks like AMDGPU changes are neutral too.
And now that i think about it, the AArch64 regression should be solvable (hidable) by an inverse transform.
Should i look into that before or after this patch?

Sat, May 18, 3:45 PM · Restricted Project
lebedev.ri updated subscribers of D62100: [DAGCombine][X86][AMDGPU][AArch64] (srl (shl x, c1), c2) with c1 != c2 handling.

Looked at changes:

  • I'll leave x86 vector stuff for later. since i actually wanted to look into reverse trasnform, and looked into this only for consistency.
  • I don't know what to do with AArch64 regression. I can hide it with shouldFoldConstantShiftPairToMask(), but it is there regardless (tests added). Thoughts?
  • That leaves AMDGPU?
Sat, May 18, 3:18 PM · Restricted Project
lebedev.ri abandoned D62102: [AArch64] Bit-extract with shifted mask - shr+and (UBFX).

Hmm, this isn't it..
Something much more fine-grained missing.

Sat, May 18, 2:15 PM · Restricted Project
lebedev.ri committed rG1a5d623ded8e: [NFC][AArch64] Autogenerate fcopysign.ll test (authored by lebedev.ri).
[NFC][AArch64] Autogenerate fcopysign.ll test
Sat, May 18, 1:25 PM
lebedev.ri retitled D62102: [AArch64] Bit-extract with shifted mask - shr+and (UBFX) from [AArch64] Bit-extract with shifted mask (UBFX) to [AArch64] Bit-extract with shifted mask - shr+and (UBFX).
Sat, May 18, 12:58 PM · Restricted Project
lebedev.ri created D62102: [AArch64] Bit-extract with shifted mask - shr+and (UBFX).
Sat, May 18, 12:37 PM · Restricted Project
lebedev.ri committed rG13ac317e4cf9: [NFC][AArch64] Autogenerate bitfield-insert.ll, selectcc-to-shiftand.ll tests (authored by lebedev.ri).
[NFC][AArch64] Autogenerate bitfield-insert.ll, selectcc-to-shiftand.ll tests
Sat, May 18, 10:44 AM
lebedev.ri committed rGd1be3c446ef8: [NFC][AArch64] Add some ubfx tests with immediates (authored by lebedev.ri).
[NFC][AArch64] Add some ubfx tests with immediates
Sat, May 18, 6:48 AM
lebedev.ri created D62100: [DAGCombine][X86][AMDGPU][AArch64] (srl (shl x, c1), c2) with c1 != c2 handling.
Sat, May 18, 6:28 AM · Restricted Project
lebedev.ri committed rG98092f37d0dd: UpdateTestChecks: fix AMDGPU handling (authored by lebedev.ri).
UpdateTestChecks: fix AMDGPU handling
Sat, May 18, 5:58 AM
lebedev.ri committed rG822b9c971be6: UpdateTestChecks: arm64-eabi handlind (authored by lebedev.ri).
UpdateTestChecks: arm64-eabi handlind
Sat, May 18, 5:58 AM
lebedev.ri added a comment to D62097: UpdateTestChecks: arm64-eabi.

LGTM

Sat, May 18, 5:56 AM · Restricted Project
lebedev.ri added inline comments to D62099: UpdateTestChecks: fix AMDGPU handling.
Sat, May 18, 5:48 AM · Restricted Project
lebedev.ri added a child revision for D62097: UpdateTestChecks: arm64-eabi: D62099: UpdateTestChecks: fix AMDGPU handling.
Sat, May 18, 2:58 AM · Restricted Project
lebedev.ri added a parent revision for D62099: UpdateTestChecks: fix AMDGPU handling: D62097: UpdateTestChecks: arm64-eabi.
Sat, May 18, 2:58 AM · Restricted Project
lebedev.ri created D62099: UpdateTestChecks: fix AMDGPU handling.
Sat, May 18, 2:57 AM · Restricted Project
lebedev.ri updated the diff for D62097: UpdateTestChecks: arm64-eabi.

Upload correct diff this time

Sat, May 18, 2:33 AM · Restricted Project
lebedev.ri created D62097: UpdateTestChecks: arm64-eabi.
Sat, May 18, 2:25 AM · Restricted Project

Fri, May 17

lebedev.ri added a comment to D62025: Expand llvm.is.constant earlier.

2c:
Is it guaranteed that this transform (instsimplify) will *ONLY* *EVER* run once at the end of the pipeline?
*Every* pipeline ever created? That is the 'requirement' on this @llvm.is.constant expansion, i believe.
Would it not be better to not rely on this implicit, undocumented, untested behavior?

Fri, May 17, 1:54 PM · Restricted Project
lebedev.ri added a comment to D62067: [Support] Time profiler: support new PassManager.

I know this is in line with what is done in old-pm, but i find the middle-end names to be very useless.
In particular, why is the actual optimization pass name is always in the description, and the actual timeslice name is always the same generic "Opt???"?
IMHO the actual optimization pass name should be the Name, not Detail.

We can change this in a separate commits in both new and old pm.

Fri, May 17, 9:11 AM · Restricted Project
lebedev.ri committed rG64c756b9917c: [DAGCombiner] visitShiftByConstant(): drop bogus signbit check (authored by lebedev.ri).
[DAGCombiner] visitShiftByConstant(): drop bogus signbit check
Fri, May 17, 8:51 AM
lebedev.ri committed rG3275060fe838: [InstCombine] canShiftBinOpWithConstantRHS(): drop bogus signbit check (authored by lebedev.ri).
[InstCombine] canShiftBinOpWithConstantRHS(): drop bogus signbit check
Fri, May 17, 8:51 AM
lebedev.ri added a comment to D62067: [Support] Time profiler: support new PassManager.

I know this is in line with what is done in old-pm, but i find the middle-end names to be very useless.
In particular, why is the actual optimization pass name is always in the description, and the actual timeslice name is always the same generic "Opt???"?
IMHO the actual optimization pass name should be the Name, not Detail.

Fri, May 17, 8:32 AM · Restricted Project
lebedev.ri added a comment to D61918: [DAGCombiner] visitShiftByConstant(): drop bogus(?) signbit check.

This logic was copied from canShiftBinOpWithConstantRHS in InstCombineShifts.cpp

Aha, that makes sense. Posted D61938.

Fri, May 17, 12:40 AM · Restricted Project

Thu, May 16

lebedev.ri added a comment to D61144: [LoopIdiomRecognize] BCmp loop idiom recognition.

ping

Thu, May 16, 3:08 PM · Restricted Project
lebedev.ri updated the diff for D61918: [DAGCombiner] visitShiftByConstant(): drop bogus(?) signbit check.

Now with tests.

Thu, May 16, 5:48 AM · Restricted Project
lebedev.ri committed rG62650cf464de: [NFC] Fixup FileCheck option name in tests added in rL360881 (authored by lebedev.ri).
[NFC] Fixup FileCheck option name in tests added in rL360881
Thu, May 16, 5:40 AM
lebedev.ri committed rGec6608d54711: [NFC][CodeGen] Add some more tests for pulling binops through shifts (authored by lebedev.ri).
[NFC][CodeGen] Add some more tests for pulling binops through shifts
Thu, May 16, 5:25 AM

Wed, May 15

lebedev.ri added a comment to D61938: [InstCombine] canShiftBinOpWithConstantRHS(): drop bogus(?) signbit check.

I have manually checked these changed tests in alive, and they are all good.

Wed, May 15, 2:36 PM · Restricted Project
lebedev.ri updated the diff for D61938: [InstCombine] canShiftBinOpWithConstantRHS(): drop bogus(?) signbit check.

Added proper full test coverage.

Wed, May 15, 2:31 PM · Restricted Project
lebedev.ri committed rG4b77a6a55ec2: [NFC][InstCombine] Add some more tests for pulling binops through shifts (authored by lebedev.ri).
[NFC][InstCombine] Add some more tests for pulling binops through shifts
Wed, May 15, 2:14 PM
lebedev.ri added a comment to D61409: [SimplifyCFG] Added condition assumption for unreachable blocks.

Just to be clear, I didn't mean to say that we shouldn't do this, just that it is not guaranteed to be a win in the end. If it's possible to do some end-to-end performance testing with this patch to make sure that it doesn't come with unexpected regressions, I see no reason not to move forward with this change. The problem with assumes is a long-standing one and I don't think it has any simple solution.

Wed, May 15, 2:09 PM · Restricted Project
lebedev.ri added a comment to D61409: [SimplifyCFG] Added condition assumption for unreachable blocks.

Any advices about next steps for this patch?

if inserted llvm.assume is a big problem and “fixing” oneUse match check is not proper solution, then this patch is dead..

In general, automatically llvm.assume much be done with extreme care, as it can pessimize optimizations. We try do this only in cases where the user as specifically indicated that there is some information relevant to optimization that is worth preserving.

Wed, May 15, 1:35 PM · Restricted Project
lebedev.ri accepted D61870: Make cl::HideUnrelatedOptionsless error-prone.

Looks good to me, but please wait for @hintonda / @beanz.

Wed, May 15, 5:17 AM · Restricted Project
lebedev.ri updated the summary of D61938: [InstCombine] canShiftBinOpWithConstantRHS(): drop bogus(?) signbit check.
Wed, May 15, 4:33 AM · Restricted Project
lebedev.ri added a comment to D61918: [DAGCombiner] visitShiftByConstant(): drop bogus(?) signbit check.

This logic was copied from canShiftBinOpWithConstantRHS in InstCombineShifts.cpp

Wed, May 15, 4:00 AM · Restricted Project
lebedev.ri created D61938: [InstCombine] canShiftBinOpWithConstantRHS(): drop bogus(?) signbit check.
Wed, May 15, 3:58 AM · Restricted Project
lebedev.ri committed rGda08fae39709: [NFC][InstCombine] Regenerate trunc.ll test (authored by lebedev.ri).
[NFC][InstCombine] Regenerate trunc.ll test
Wed, May 15, 3:25 AM

Tue, May 14

lebedev.ri added reviewers for D61918: [DAGCombiner] visitShiftByConstant(): drop bogus(?) signbit check: jojo, rengolin.

(adding reviewers from D27916)

Tue, May 14, 2:52 PM · Restricted Project
lebedev.ri created D61918: [DAGCombiner] visitShiftByConstant(): drop bogus(?) signbit check.
Tue, May 14, 2:44 PM · Restricted Project
lebedev.ri committed rG7baf528aba22: [NFC][CodeGen][X86][AArch64] Add and-const-mask + const-shift pattern tests (authored by lebedev.ri).
[NFC][CodeGen][X86][AArch64] Add and-const-mask + const-shift pattern tests
Tue, May 14, 1:17 PM
lebedev.ri added inline comments to D61887: [SelectionDAG] computeKnownBits - support constant pool values from target.
Tue, May 14, 3:53 AM · Restricted Project
lebedev.ri added a comment to D61243: Check build-type dependent variables for -flto as well..

https://github.com/Kitware/CMake/blob/c4b4d8b3a67718e29edb5676273e528dab566672/Modules/CMakeCXXInformation.cmake#L268-L271
so i'd really recommend to try to always set it, and see if that works

Tue, May 14, 3:28 AM · Restricted Project
lebedev.ri added a comment to D61479: Finish "Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO".

clang codegen tests missing.
The problem with such things is that it got landed without review, and with zero tests.
And now you've got a feature that has no test coverage (== may have gotten broken in meantime), and a stuck patch that might add tests for it..

Tue, May 14, 3:03 AM · Restricted Project, Restricted Project
lebedev.ri added inline comments to D61643: [PragmaHandler][NFC] Expose `#pragma` location.
Tue, May 14, 3:00 AM · Restricted Project
lebedev.ri accepted D61794: [LVI][CVP] Add support for abs/nabs select pattern flavor.

This looks good to me, but would be good if someone else familiar with CVP could review.

Tue, May 14, 3:00 AM · Restricted Project
lebedev.ri added inline comments to D61851: [clang-tidy] New option for misc-throw-by-value-catch-by-reference.
Tue, May 14, 2:21 AM · Restricted Project, Restricted Project, Restricted Project
lebedev.ri accepted D61830: [X86][SSE] Disable shouldFoldConstantShiftPairToMask for scalar shifts on AMD targets (PR40758).

LG
I'm not sure why [SSE] tag is in the subject, should it be [AMD]?

Tue, May 14, 2:19 AM · Restricted Project

Mon, May 13

lebedev.ri added inline comments to D61830: [X86][SSE] Disable shouldFoldConstantShiftPairToMask for scalar shifts on AMD targets (PR40758).
Mon, May 13, 2:15 PM · Restricted Project
lebedev.ri added a comment to D61870: Make cl::HideUnrelatedOptionsless error-prone.

Thanks! I do like the llvm-cat -help changes, and this does make sense in that context.
@hintonda can you comment on why the lhs of the diff is the way it is, why these were cl::ReallyHidden?

Mon, May 13, 2:08 PM · Restricted Project
lebedev.ri added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

Sounds good, thank you!

Mon, May 13, 1:59 PM · Restricted Project, Restricted Project
lebedev.ri added inline comments to D24892: [clang-tidy] Add option "LiteralInitializers" to cppcoreguidelines-pro-type-member-init.
Mon, May 13, 12:55 PM · Restricted Project

Sun, May 12

lebedev.ri added a reviewer for D59480: [NFC] Add SchedState to allow forwarding the Scheduling state between MBB: andreadb.
Sun, May 12, 11:21 PM · Restricted Project
lebedev.ri added a reviewer for D61248: [NFC] Add the infrastructure to forward the scheduled state between MBB: andreadb.
Sun, May 12, 11:21 PM · Restricted Project
lebedev.ri added reviewers for D61843: [DAGCombine] Match a pattern where a wide type scalar value is stored by several narrow stores: lebedev.ri, spatel.

This looks like a middle-end optimization problem to me.
https://godbolt.org/z/bwR-k1
Final i64 is certainly a legal type as per datalayout, and that IR certainly doesn't loop optimal to me.

Sun, May 12, 11:15 PM · Restricted Project
lebedev.ri added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

When I asked for a test above, I understood you to say you couldn't provide one, but If I misunderstood, by all means, please add the test.

Please do note that i have provided a testcase (godbolt link) in my very first comment, and quoted that line when replying the previous time.
(Granted, that loop is not in a correct form for openmp, but the point being, the current check does not diagnose it either)

I'm really not sure I understand what you are trying to say.

Sun, May 12, 2:12 PM · Restricted Project, Restricted Project
lebedev.ri added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

As a general rule, don't add anything that doesn't include a test.

Since this "false positive" is apparently untestable,

How so?

When I asked for a test above, I understood you to say you couldn't provide one, but If I misunderstood, by all means, please add the test.

Sun, May 12, 1:25 PM · Restricted Project, Restricted Project
lebedev.ri added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

As a general rule, don't add anything that doesn't include a test.

Since this "false positive" is apparently untestable,

How so?

Sun, May 12, 1:15 PM · Restricted Project, Restricted Project
lebedev.ri added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

Yes and no.
This will prevent the false-positive, but only if the OpenMP is enabled (-fopenmp).
If OpenMP is not enabled, then that won't work, because there won't be anything about OpenMP in AST.
I semi-strongly believe it will be less confusing to only document this false-positive (in check's docs),
instead of trying to prevent it, and reliably failing in half of the cases.

Sun, May 12, 1:00 PM · Restricted Project, Restricted Project

Sat, May 11

lebedev.ri added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

Sat, May 11, 1:07 PM · Restricted Project, Restricted Project
lebedev.ri added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

Sat, May 11, 12:42 PM · Restricted Project, Restricted Project
lebedev.ri added reviewers for D61827: [clang-tidy] modernize-loop-convert: impl const cast iter: aaron.ballman, JonasToth.
Sat, May 11, 12:42 PM · Restricted Project, Restricted Project

Fri, May 10

lebedev.ri added a comment to D61809: [BPF] Preserve debuginfo array/union/struct type name/access index.

Tests seems to be missing?

Fri, May 10, 3:42 PM · Restricted Project
lebedev.ri added a comment to D61810: [BPF] add new intrinsics preserve_{array,union,struct}_access_index.

Missing langref changes.
What about other targets, does this need some default expansion?

Fri, May 10, 3:41 PM · Restricted Project
lebedev.ri added a comment to D61749: [clang-tidy] initial version of readability-static-const-method.

...

Still, this is two separate checks, logically.
You can put the FindUsageOfThis e.g. into a new file in clang-tidy/utils.

Fri, May 10, 1:47 PM · Restricted Project, Restricted Project
lebedev.ri added a reviewer for D61749: [clang-tidy] initial version of readability-static-const-method: lebedev.ri.

@JonasToth tried to implement const check, so probably const and static part should be split.

I agree, there is D54943, which should be 99% ready to go, and is only stuck due to @JonasToth
time constraints, IIRC. Treating *this pointer should simply be a logical extension of
that check. (unless it already does that, or there is some other patch i forgot about)

Fri, May 10, 10:57 AM · Restricted Project, Restricted Project
lebedev.ri updated the summary of D61749: [clang-tidy] initial version of readability-static-const-method.
Fri, May 10, 10:50 AM · Restricted Project, Restricted Project
lebedev.ri updated the summary of D61749: [clang-tidy] initial version of readability-static-const-method.
Fri, May 10, 10:48 AM · Restricted Project, Restricted Project
lebedev.ri added a comment to D61660: [cmake] Make google benchmark project handle libraries properly when built in-tree.

I think i'm failing to convey the thoughts here.
Can you explain why you insist on having

if(COMMAND llvm_add_library)
  function(benchmark_add_library target)
    llvm_add_library(${target} ${ARGN})
  endfunction()
else()
  function(benchmark_add_library target)
    add_library(${target} ${ARGN})
  endfunction()
endif()

in llvm/utils/benchmark/CMakeLists.txt?

Sorry, I seem to have touched a nerve, and certainly didn't mean to offend
anyone. As a volunteer, my only goal is to do what I can to improve the
project.

I'm sorry, the problem is most quite likely on my side indeed,
I'n *not* offended, i was just not observing the logic about *this specific layout* for the change.
I agree that the change is needed.

Fri, May 10, 10:30 AM · Restricted Project

Thu, May 9

lebedev.ri committed rG9db0e72570f7: [X86] AMD Piledriver (BdVer2): major cleanup (mainly inverse throughput) (authored by lebedev.ri).
[X86] AMD Piledriver (BdVer2): major cleanup (mainly inverse throughput)
Thu, May 9, 6:53 AM
lebedev.ri added a comment to D61660: [cmake] Make google benchmark project handle libraries properly when built in-tree.

I think i'm failing to convey the thoughts here.
Can you explain why you insist on having

if(COMMAND llvm_add_library)
  function(benchmark_add_library target)
    llvm_add_library(${target} ${ARGN})
  endfunction()
else()
  function(benchmark_add_library target)
    add_library(${target} ${ARGN})
  endfunction()
endif()

in llvm/utils/benchmark/CMakeLists.txt?

Thu, May 9, 4:11 AM · Restricted Project
lebedev.ri committed rGa8f8d3b01e94: Revert "[OPENMP]Fix PR41768: check DSA for globals with `default(none)` clauses. (authored by lebedev.ri).
Revert "[OPENMP]Fix PR41768: check DSA for globals with `default(none)` clauses.
Thu, May 9, 3:50 AM
lebedev.ri committed rGb32a02b5bc6a: Revert "[OPENMP]Fix PR41767: diagnose DSA for variables in clauses with default… (authored by lebedev.ri).
Revert "[OPENMP]Fix PR41767: diagnose DSA for variables in clauses with default…
Thu, May 9, 3:46 AM

Wed, May 8

lebedev.ri added inline comments to D61693: Rough out InstCombine::visitFNeg(...).
Wed, May 8, 12:42 PM · Restricted Project
lebedev.ri added inline comments to D61660: [cmake] Make google benchmark project handle libraries properly when built in-tree.
Wed, May 8, 10:00 AM · Restricted Project
lebedev.ri requested changes to D61660: [cmake] Make google benchmark project handle libraries properly when built in-tree.

.. this doesn't look like what i proposed, and has some unrelated diff?

It uses your wrapper suggestion, but keeps everything local to benchmark. Generally, if projects are build within other projects, e.g., see clang/CMakeLists.txt, they don't redeclare project(). Doing so wipes out some state, most notably policies. By checking, you preserve whatever the parent project set.

Wed, May 8, 9:37 AM · Restricted Project
lebedev.ri added a comment to D61660: [cmake] Make google benchmark project handle libraries properly when built in-tree.

.. this doesn't look like what i proposed, and has some unrelated diff?

Wed, May 8, 9:25 AM · Restricted Project
lebedev.ri added inline comments to D61680: [X86] Avoid SFB - Fix inconsistent codegen with/without debug info .
Wed, May 8, 7:11 AM · Restricted Project
lebedev.ri added a comment to D61680: [X86] Avoid SFB - Fix inconsistent codegen with/without debug info .

Hm, can -debugify be run by llc?

Wed, May 8, 7:11 AM · Restricted Project

Tue, May 7

lebedev.ri added a comment to D61660: [cmake] Make google benchmark project handle libraries properly when built in-tree.

Let's make this a bit more upstreamable:
Add this to llvm/CMakeLists.txt

function(benchmark_add_library target)
  llvm_add_library(${target} ${ARGN})
endfunction()
Tue, May 7, 11:19 PM · Restricted Project
lebedev.ri added reviewers for D61643: [PragmaHandler][NFC] Expose `#pragma` location: rsmith, aaron.ballman.

Seems like obvious NFC to me, but i'm not sure about API/ABI implications for external use.

Tue, May 7, 10:03 AM · Restricted Project
lebedev.ri added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

I don't see why this differential can't be updated to only contain the remaining part
of the diff (for the actual OpenMP change), after splitting the NFC refactoring part.

Tue, May 7, 9:26 AM · Restricted Project
lebedev.ri added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

It would have been better to submit this refactor as a new patch..

Sorry, I didn't realize that was the norm. I can do that now if it would help. I can also revert changes to this patch if the goal is to make it easier to reference the old version.

Tue, May 7, 9:17 AM · Restricted Project
lebedev.ri added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

It would have been better to submit this refactor as a new patch..

Tue, May 7, 9:09 AM · Restricted Project
lebedev.ri added a comment to D61307: [InstCombine] Add new combine to sub folding.

Was this committed already?

Tue, May 7, 8:32 AM · Restricted Project
lebedev.ri accepted D61517: [InstCombine] Add new combine to add folding..

Looks reasonable.

Tue, May 7, 8:29 AM · Restricted Project
lebedev.ri committed rG9bac7d8165dd: [llvm-exegesis] BenchmarkRunner::runConfiguration(): write small snippet to… (authored by lebedev.ri).
[llvm-exegesis] BenchmarkRunner::runConfiguration(): write small snippet to…
Tue, May 7, 5:30 AM
lebedev.ri committed rG724a68f372c0: [llvm-exegesis] InstructionBenchmark::writeYamlTo(): don't forget to flush() (authored by lebedev.ri).
[llvm-exegesis] InstructionBenchmark::writeYamlTo(): don't forget to flush()
Tue, May 7, 2:20 AM

Sun, May 5

lebedev.ri updated the diff for D61144: [LoopIdiomRecognize] BCmp loop idiom recognition.

Rebased, ping, any thoughts, no matter how high-level?

Sun, May 5, 11:50 PM · Restricted Project
lebedev.ri added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

  • I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
  • This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.

    I see two paths forward:
  • Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
  • Moar abstractions - how about not changing the startloc of openmp directives,

My alternative proposal was exactly that. A difficulty is how to pass the #pragma location to the OpenMP AST node constructors. PragmaOpenMPHandler::HandlePragma passes locations via the tok::annot_pragma_openmp and tok::annot_pragma_openmp_end tokens, so where do we pass this new location? I proposed creating a third token, and Alexey was concerned over the parsing problems that would create.

but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?

I agree that a base class would be a nice way to extend all pragma classes with the PragmaIntroducer.

I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.

One way to avoid creating an extra token would be to widen the Token class to store the additional location. The Token documentation says it's not intended to be space efficient. How does that sound to people?

Sun, May 5, 11:17 PM · Restricted Project