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 (325 w, 1 d)

Recent Activity

Today

lebedev.ri added an edge to rCTE351686: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit…: D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods.
Sun, Jan 20, 6:34 AM
lebedev.ri added 1 commit(s) for D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods: rCTE351686: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit….
Sun, Jan 20, 6:34 AM
lebedev.ri added inline comments to D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods.
Sun, Jan 20, 6:12 AM
lebedev.ri added inline comments to D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods.
Sun, Jan 20, 5:57 AM
lebedev.ri accepted D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods.

LG other than two nits, thank you!

Sun, Jan 20, 5:54 AM

Yesterday

lebedev.ri requested changes to D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods.

Some nits.

Sat, Jan 19, 1:15 PM

Fri, Jan 18

lebedev.ri added a comment to D54318: [WIP][llvm-exegesis] Add support for memory instructions in latency mode..

Is something like this still planned?
Looks like this 'kinda' stalled.

Fri, Jan 18, 10:31 AM
lebedev.ri added a comment to D56819: Document toolchain update policy.

Looks good to me too. (don't think i should formally accept this though)

Fri, Jan 18, 9:48 AM

Thu, Jan 17

lebedev.ri set the repository for D56851: [ASTMatchers] Adds `CXXMemberCallExpr` matcher `invokedAtType`. to rC Clang.
Thu, Jan 17, 5:57 AM
lebedev.ri updated subscribers of D56851: [ASTMatchers] Adds `CXXMemberCallExpr` matcher `invokedAtType`..

Please always subscribe lists.
The reviews that happen without lists are null and void.

Thu, Jan 17, 5:57 AM
lebedev.ri set the repository for D56850: [ASTMatchers][NFC] Add tests for assorted `CXXMemberCallExpr` matchers. to rC Clang.
Thu, Jan 17, 5:57 AM
lebedev.ri updated subscribers of D56849: [ASTMatchers][NFC] Update comments on assorted `CXXMemberCallExpr` matchers..

Please always subscribe lists.
The reviews that happen without lists are null and void.

Thu, Jan 17, 5:57 AM
lebedev.ri updated subscribers of D56850: [ASTMatchers][NFC] Add tests for assorted `CXXMemberCallExpr` matchers..

Please always subscribe lists.
The reviews that happen without lists are null and void.

Thu, Jan 17, 5:57 AM
lebedev.ri retitled D56849: [ASTMatchers][NFC] Update comments on assorted `CXXMemberCallExpr` matchers. from [ASTMatchers] Update comments on assorted `CXXMemberCallExpr` matchers. to [ASTMatchers][NFC] Update comments on assorted `CXXMemberCallExpr` matchers..
Thu, Jan 17, 5:57 AM

Wed, Jan 16

lebedev.ri added a parent revision for D56811: [Mem2Reg] Enable promotion for bitcastable load/store values: D56810: [Mem2Reg] Enable promotion for bitcastable load/store values.
Wed, Jan 16, 2:23 PM
lebedev.ri added a child revision for D56810: [Mem2Reg] Enable promotion for bitcastable load/store values: D56811: [Mem2Reg] Enable promotion for bitcastable load/store values.
Wed, Jan 16, 2:23 PM
lebedev.ri added a comment to D56811: [Mem2Reg] Enable promotion for bitcastable load/store values.

The diff does not seem to match the subject.
(also, probably should go do llvm-commits)

Wed, Jan 16, 2:23 PM
lebedev.ri added a comment to D56786: [ASTMatchers] Changes to `CXXMemberExpr` matchers..

Can you break this up into multiple commits?

Sure, but any suggestions on granularity? E.g. i can split into two: the fixes/clarifications in one and the new matcher in another; or i could split into four -- one for each bullet, etc.

Wed, Jan 16, 8:36 AM
lebedev.ri retitled D56786: [ASTMatchers] Changes to `CXXMemberExpr` matchers. from Changes to `CXXMemberExpr` matchers. to [ASTMatchers] Changes to `CXXMemberExpr` matchers..
Wed, Jan 16, 8:33 AM
lebedev.ri added inline comments to D56082: [X86][SLP] Enable SLP vectorization for 128-bit horizontal X86 instructions (add, sub).
Wed, Jan 16, 3:22 AM

Tue, Jan 15

lebedev.ri added a comment to D56738: [SelectionDAG] Update check in createOperands to reflect max() is a valid value..

Can any test case be added here, or would it be just too big to realistically test?

Tue, Jan 15, 11:59 AM
lebedev.ri added a comment to D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419).

LGTM

Tue, Jan 15, 11:49 AM
lebedev.ri added inline comments to D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419).
Tue, Jan 15, 11:38 AM
lebedev.ri updated the diff for D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419).

Address review feedback - use X.hasOneUse() directly.

Tue, Jan 15, 11:38 AM
lebedev.ri updated the diff for D56715: X86DAGToDAGISel::matchBitExtract(): prepare 'control' in 32 bits.

Rebased, NFC.

Tue, Jan 15, 11:38 AM
lebedev.ri added a comment to D56103: Add lock function definitions to fix Bug 40042.

Hmm, thanks, i missed the patch, and wasn't CC'd here.
I will check if that silences the ubsan.

Tue, Jan 15, 11:24 AM · Restricted Project
lebedev.ri added inline comments to D56731: Add -Wctad-maybe-unsupported to diagnose CTAD on types with no user defined deduction guides..
Tue, Jan 15, 10:27 AM
lebedev.ri added a child revision for D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419): D56715: X86DAGToDAGISel::matchBitExtract(): prepare 'control' in 32 bits.
Tue, Jan 15, 4:53 AM
lebedev.ri added a parent revision for D56715: X86DAGToDAGISel::matchBitExtract(): prepare 'control' in 32 bits: D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419).
Tue, Jan 15, 4:53 AM
lebedev.ri created D56715: X86DAGToDAGISel::matchBitExtract(): prepare 'control' in 32 bits.
Tue, Jan 15, 4:53 AM
lebedev.ri added a comment to D56646: [OpenCL] opencl-c.h: read_image*(): sampler-less, and image{1,2}d_array_t variants are OpenCL-1.2+, mark them as such.

LGTM!

Thank you for the review.

Tue, Jan 15, 3:19 AM · Restricted Project
lebedev.ri updated the diff for D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419).

Only skip one-use truncations.

Tue, Jan 15, 2:47 AM
lebedev.ri updated the diff for D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419).

Rebased ontop of patch with extra-use trunk.
Did not add the use check yet..

Tue, Jan 15, 2:43 AM
lebedev.ri added a comment to D56424: [clang-tidy] Add check for underscores in googletest names..

Hi all, ping on this patch. I've addressed all comments to the best of my ability. Is there anything outstanding that needs to be changed?

Round about this time of a review we normally hear @JonasToth asking if you've run this on a large C++ code base like LLVM (with fix-its), and seen if the project still builds afterwards..might be worth doing that ahead of time if you haven't done so already

Tue, Jan 15, 12:12 AM · Restricted Project

Mon, Jan 14

lebedev.ri added a comment to D56679: [InstCombine] Don't undo 0 - (X * Y) canonicalization when combining subs..

Looks good, no further comments from me.

Mon, Jan 14, 11:26 PM
lebedev.ri added a comment to D56277: AggressiveInstCombine: Add tests for full multiplication pattern match.

Right. The existing tests seem ok.
Probably some negative tests will be needed though.

Mon, Jan 14, 11:26 PM
lebedev.ri added inline comments to D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419).
Mon, Jan 14, 2:23 PM
lebedev.ri added a comment to D56679: [InstCombine] Don't undo 0 - (X * Y) canonicalization when combining subs..

We are sure that we want to break the cycle here, and not at the other end?
This results in the optimal IR?

Mon, Jan 14, 11:57 AM
lebedev.ri added inline comments to D54590: [compiler-rt][UBSan] Sanitization for alignment assumptions..
Mon, Jan 14, 11:52 AM · Restricted Project
lebedev.ri added inline comments to D54590: [compiler-rt][UBSan] Sanitization for alignment assumptions..
Mon, Jan 14, 11:10 AM · Restricted Project
lebedev.ri updated the diff for D54589: [clang][UBSan] Sanitization for alignment assumptions..

Rebased before commit, NFC.

Mon, Jan 14, 11:07 AM · Restricted Project
lebedev.ri updated the diff for D54588: [llvm][IRBuilder] Introspection for CreateAlignmentAssumption*() functions.

Rebased before commit, NFC.

Mon, Jan 14, 11:07 AM
lebedev.ri updated the diff for D54590: [compiler-rt][UBSan] Sanitization for alignment assumptions..

Rebased before commit, addressed @morehouse review note.

Mon, Jan 14, 11:07 AM · Restricted Project
lebedev.ri updated subscribers of D54590: [compiler-rt][UBSan] Sanitization for alignment assumptions..

Oh yay, thank you for the review!

Mon, Jan 14, 10:32 AM · Restricted Project
lebedev.ri retitled D56241: [Sema] expose a control flag for integer to pointer ext warning from expose a control flag for interger to pointer ext warning to [Sema] expose a control flag for integer to pointer ext warning.
Mon, Jan 14, 9:28 AM · Restricted Project
lebedev.ri added a comment to D56201: [LegalizeVectorTypes] Allow single loads and stores for more short vectors.

I modified some AMDGPU tests to track more registers where possible as @jvesely suggested, and I added some missing new relevant generated instructions (BFE_INT).

I can't comment on the x86 ones.

Mon, Jan 14, 9:24 AM
lebedev.ri added a comment to D40988: Clang-format: add finer-grained options for putting all arguments on one line.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options comes into mind.

Mon, Jan 14, 8:23 AM
lebedev.ri added a comment to D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options comes into mind.

Mon, Jan 14, 8:23 AM · Restricted Project
lebedev.ri added a comment to D56605: Implement isSafeToPropagatePtrEquality.

Same, tests.
Also, all these differentials don't have appropriate lists subscribed, so no review can happen.

Mon, Jan 14, 4:08 AM
lebedev.ri added a comment to D56601: Update CaptureTracker so psub does not capture operands if possible.

Tests missing.

Mon, Jan 14, 4:07 AM
lebedev.ri added a comment to D56598: Add llvm.psub.

Tests missing.

Mon, Jan 14, 4:05 AM

Sun, Jan 13

lebedev.ri added a comment to D55783: [OpenMP] Fix LIBOMP_USE_DEBUGGER=ON build (PR38612).

LGTM

Sun, Jan 13, 4:53 AM · Restricted Project
lebedev.ri added inline comments to D56644: [clang-tidy] readability-container-size-empty handle std::string length().
Sun, Jan 13, 3:22 AM · Restricted Project
lebedev.ri added a comment to D56644: [clang-tidy] readability-container-size-empty handle std::string length().

Seems to look good.

Sun, Jan 13, 3:06 AM · Restricted Project
lebedev.ri created D56646: [OpenCL] opencl-c.h: read_image*(): sampler-less, and image{1,2}d_array_t variants are OpenCL-1.2+, mark them as such.
Sun, Jan 13, 2:54 AM · Restricted Project

Sat, Jan 12

lebedev.ri added a comment to D56644: [clang-tidy] readability-container-size-empty handle std::string length().

For now, I just added tests. I have several questions, as I'm beginner (ContainerSizeEmptyCheck.cpp).

  1. Do I have to extend ValidContainer, so it recognises ::std::string with length() method as valid container too or we can assume ::std::string as valid container by default?
  2. If ::std::string is valid container, I just add one more expression to match. Is it correct way?
Sat, Jan 12, 11:51 PM · Restricted Project
lebedev.ri added a comment to D56644: [clang-tidy] readability-container-size-empty handle std::string length().

Either this is a NFC change with just tests, or the actual fix is missing.

Sat, Jan 12, 11:32 PM · Restricted Project

Fri, Jan 11

lebedev.ri added a comment to D47073: Document and Enforce new Host Compiler Policy.

Looks good, that is more in line with what i have been suggesting in IRC.
Some nits:

Fri, Jan 11, 10:43 AM

Thu, Jan 10

lebedev.ri added a comment to D54590: [compiler-rt][UBSan] Sanitization for alignment assumptions..

ping.

Thu, Jan 10, 10:03 AM · Restricted Project
lebedev.ri added a comment to D56242: Elevate instructions across if-else blocks for better constant propagation.

Please include a few simpler testcases, so it's more obvious what the pass actually does. As far as I can tell, this is similar to InstCombiner::foldOpIntoPhi, but it's a lot more aggressive?

I feel like pointing out that a "separate IR hoisting pass" has been brought up several times in previous reviews, as the means to potentially drop some stuff out of instcombine e.g.

Thu, Jan 10, 9:39 AM

Tue, Jan 8

lebedev.ri added a comment to D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419).

Hm, ping.
I'm guessing everyone is busy with funnel-related stuff, on the eve of the 8.0
Or would it be easier if i dropped all the TLDR in the description?

Tue, Jan 8, 4:27 AM

Mon, Jan 7

lebedev.ri added a comment to D56424: [clang-tidy] Add check for underscores in googletest names..

Clean up comments in test file.

Mon, Jan 7, 11:34 PM · Restricted Project
lebedev.ri retitled D56424: [clang-tidy] Add check for underscores in googletest names. from Add check for underscores in googletest names. to [clang-tidy] Add check for underscores in googletest names..
Mon, Jan 7, 11:33 PM · Restricted Project
lebedev.ri added a comment to D54742: [CodeMetrics] Don't let extends of i1 be free..

Differential's name is slightly misleading.
Extends from i1 weren't all free before, only if they were an extensions of an icmp result.

Mon, Jan 7, 5:58 AM
lebedev.ri added inline comments to D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr .
Mon, Jan 7, 5:12 AM · Restricted Project
lebedev.ri accepted D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr .

LG in principle.

Mon, Jan 7, 1:25 AM · Restricted Project

Sun, Jan 6

lebedev.ri added a comment to D56242: Elevate instructions across if-else blocks for better constant propagation.

Yes, it actually is so. But since there was already a pass by that name, I thought I'd name mine differently so as not to confuse one with the other. Any suggestions to rename the new pass is most welcome.

I would personally think one wouldn't want to end up with dozens of similar passes doing essentially the same thing, but just slightly differently.
Have you looked at the existing pass(es)? How are they different from this code? Perhaps it would be best to extend the existing passes?

Yes, I too am of the same opinion. However, the other two passes that hoist code are ConstantHoisting and GVNHoisting. While the former hoists constants the latter hoists expressions from branches to the common dominator. What the Elevate pass does is to hoist expressions from a join block to its predecessors if the expressions can be constant evaluated. (In effect, it does the reverse of what the Sink pass does - to sink expressions into the join block from its predecessors).

All that is a very good point, and i think it even reinforces my previous point.
I think it would be best to be consistent, and call this InstructionHoisting.

Sun, Jan 6, 11:00 AM
lebedev.ri added a comment to D56242: Elevate instructions across if-else blocks for better constant propagation.

Hm, why i'm the reviewer here?

Sorry, I somehow got the impression that you were also looking after the regression test framework and hence thought that you should review the test case part of it.

It is usually best to add the people who last worked on the newly changed code
(git diff --name-only --cached | xargs -n 1 git blame --porcelain | grep "^author " | sort | uniq -c | sort -nr | head -10),
(maybe change --cached to upstream/master..HEAD) and the code owners. (i'm neither here)

Sun, Jan 6, 6:37 AM

Sat, Jan 5

lebedev.ri added inline comments to D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr .
Sat, Jan 5, 1:14 PM · Restricted Project
lebedev.ri added reviewers for D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr : rjmccall, rsmith, erichkeane.
Sat, Jan 5, 1:09 PM · Restricted Project
lebedev.ri added a comment to D56355: [InstCombine] Simplify cttz/ctlz + icmp ugt/ult into mask check.

Hm, why does this modify the existing instruction, and manually adds it to worklist,
instead of simply creating a new instruction? This is rather unusual for instcombine i think.

I assumed that this was to avoid creating a new instruction when we can reuse an existing one.
If this is just an anachronism, I can switch the existing code to create new nodes in a separate NFC commit and then adjust it here as well.

Sat, Jan 5, 11:14 AM
lebedev.ri added a comment to D56355: [InstCombine] Simplify cttz/ctlz + icmp ugt/ult into mask check.

Hm, why does this modify the existing instruction, and manually adds it to worklist,
instead of simply creating a new instruction? This is rather unusual for instcombine i think.

Sat, Jan 5, 10:27 AM
lebedev.ri added inline comments to D56355: [InstCombine] Simplify cttz/ctlz + icmp ugt/ult into mask check.
Sat, Jan 5, 8:38 AM
lebedev.ri retitled D56354: [AST] Replace asserts with if() in SourceLocation accessors from Replace asserts with if() in SourceLocation accessors to [AST] Replace asserts with if() in SourceLocation accessors.
Sat, Jan 5, 5:41 AM

Fri, Jan 4

lebedev.ri retitled D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr from Handle member variables in readability-simplify-boolean-expr to [clang-tidy] Handle member variables in readability-simplify-boolean-expr.
Fri, Jan 4, 10:50 AM · Restricted Project
lebedev.ri requested changes to D35035: [InstCombine] Prevent memcpy generation for small data size.

Data layout question wasn't resolved. (just signalling, i don't have a preference one way or another)

Fri, Jan 4, 3:06 AM

Thu, Jan 3

lebedev.ri accepted D56241: [Sema] expose a control flag for integer to pointer ext warning.

ok, LGTM.

Thu, Jan 3, 10:06 PM · Restricted Project
lebedev.ri accepted D54450: Get the correct range of tokens for preprocessor conditions.

To me this looks like a reasonably straight-forward refactoring.
I'm guessing the initial code had good test coverage, and none of those tests break; and the new code appears to have reasonable test coverage.
(Mind you, i'm not a code owner of that code.)

Thu, Jan 3, 1:25 PM
lebedev.ri added a comment to D54590: [compiler-rt][UBSan] Sanitization for alignment assumptions..

Happy 2019 everyone!
Last ping before 8.0 branch.

Thu, Jan 3, 1:00 PM · Restricted Project
lebedev.ri added inline comments to D56241: [Sema] expose a control flag for integer to pointer ext warning.
Thu, Jan 3, 12:59 PM · Restricted Project
lebedev.ri added inline comments to D56214: AggressiveInstCombine: Fold full mul i64 x i64 -> i128.
Thu, Jan 3, 10:37 AM
lebedev.ri added inline comments to D56214: AggressiveInstCombine: Fold full mul i64 x i64 -> i128.
Thu, Jan 3, 10:27 AM
lebedev.ri added a comment to D56275: x86 interrupt calling convention: Fix argument offsets.

Test?

I tried to add additional checks to the x86-64-intrcc.ll test, but the patched version of llc generates the exact same assembly as the unpatched version for this test (with and without -O0). I think this is because the test doesn't use the relevant code paths. I'm not sure whether it's practical to construct new tests for every code path and how to construct such tests.

Thu, Jan 3, 9:44 AM
lebedev.ri added a comment to D56275: x86 interrupt calling convention: Fix argument offsets.

Test?

Thu, Jan 3, 8:48 AM
lebedev.ri added a comment to D56241: [Sema] expose a control flag for integer to pointer ext warning.

Looks ok in general.
Perhaps there should be a test for this flag specifically?
Also, should "pointer-integer-compare" be in some group?

Thu, Jan 3, 3:38 AM · Restricted Project
lebedev.ri added a comment to D56249: python compat - print statement.

Can you please submit the google benchmark diff separately, and submit it upstream at the same time, please?

Thu, Jan 3, 3:32 AM
lebedev.ri added a comment to D56248: [SelectionDAG][RFC] Allow the user to specify a memeq function..

How is this supposed to be exposed to the middle-end, front-end?
Might be better off as a RFC..

Thu, Jan 3, 1:19 AM

Wed, Jan 2

lebedev.ri resigned from D56242: Elevate instructions across if-else blocks for better constant propagation.

Hm, why i'm the reviewer here?
That being said, is "elevation" == "hoisting" here?
That might be a better, more common name..

Wed, Jan 2, 11:44 PM
lebedev.ri added a comment to D56214: AggressiveInstCombine: Fold full mul i64 x i64 -> i128.

Haven't taken a deep look yet, but some preliminary thoughts.
Also, i don't think this should be hardcoded to some particular bitwidth.

Wed, Jan 2, 12:23 PM
lebedev.ri added a comment to D55783: [OpenMP] Fix LIBOMP_USE_DEBUGGER=ON build (PR38612).

ping

Wed, Jan 2, 4:19 AM · Restricted Project

Tue, Jan 1

lebedev.ri added a comment to D56052: X86DAGToDAGISel::matchBitExtract() with truncation (PR36419).

low-key ping

Tue, Jan 1, 8:14 AM
lebedev.ri added inline comments to D56185: [BDCE] Remove instructions without demanded bits.
Tue, Jan 1, 4:08 AM
lebedev.ri added inline comments to D56185: [BDCE] Remove instructions without demanded bits.
Tue, Jan 1, 3:49 AM

Sun, Dec 30

lebedev.ri added a comment to D56160: [clang-tidy] modernize-use-trailing-return check.

Some thoughts.

Sun, Dec 30, 10:06 AM · Restricted Project
lebedev.ri retitled D56160: [clang-tidy] modernize-use-trailing-return check from created clang-tidy pass modernize-use-trailing-return to [clang-tidy] modernize-use-trailing-return check.
Sun, Dec 30, 9:47 AM · Restricted Project
lebedev.ri added a comment to D56160: [clang-tidy] modernize-use-trailing-return check.

Please always upload all patches with full context (-U99999).

Sun, Dec 30, 9:47 AM · Restricted Project
lebedev.ri retitled D54408: [ASTMatchers] Add matchers available through casting to derived from Add matchers available through casting to derived to [ASTMatchers] Add matchers available through casting to derived.
Sun, Dec 30, 5:45 AM
lebedev.ri added a comment to D54408: [ASTMatchers] Add matchers available through casting to derived.

Differential lacks description.

Sun, Dec 30, 5:45 AM

Sat, Dec 29

lebedev.ri accepted D55961: [InstCombine] canonicalize MUL with NEG operand.

LGTM, thanks.
I think this is good to go.
Not sure whether you want to wait for another review.

Sat, Dec 29, 10:14 PM
lebedev.ri added inline comments to D56155: [docs] cttz and ctlz return poison, not undef, when argument is 0.
Sat, Dec 29, 1:15 PM