Page MenuHomePhabricator

ctetreau (Christopher Tetreault)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 11 2019, 12:14 PM (66 w, 1 d)

Recent Activity

Thu, Jan 14

ctetreau added a comment to D94451: Proposal for allowing unsupported build system configuration in-tree.

Question: Would there be an expectation that unsupported build systems are in a good state for LLVM version branches? If I checkout the git tag for a final LLVM release, should I be able to expect that the build system is functional at that commit?

No. I think there should be no expectation. If the community members who care about the component can figure out a way to make this happen that doesn't make releasing harder for the release manager then they could try to arrange this, but I suspect that's not really possible. I think if that needs to be clearer, it should be clarified in the support policy. https://llvm.org/docs/SupportPolicy.html#peripheral-tier mentions things that aren't "regularly released", but I think that zero expectation of support in releases could be made clearer. Assuming that's already the status quo, I could send it as a patch to the support policy separate from this proposal. Or I could include in this proposal that we'll amend the support policy to reflect that.

Thu, Jan 14, 10:36 AM

Tue, Jan 12

ctetreau added inline comments to D94065: [NFC] Make remaining cost functions in LoopVectorize.cpp use InstructionCost.
Tue, Jan 12, 3:39 PM · Restricted Project
ctetreau accepted D94484: [NFC][InstructionCost] Use InstructionCost in Transforms/Scalar/RewriteStatepointsForGC.cpp.

lgtm

Tue, Jan 12, 3:18 PM · Restricted Project

Mon, Jan 11

ctetreau added a comment to D94451: Proposal for allowing unsupported build system configuration in-tree.

Question: Would there be an expectation that unsupported build systems are in a good state for LLVM version branches? If I checkout the git tag for a final LLVM release, should I be able to expect that the build system is functional at that commit?

Mon, Jan 11, 2:59 PM
ctetreau added inline comments to D94065: [NFC] Make remaining cost functions in LoopVectorize.cpp use InstructionCost.
Mon, Jan 11, 9:44 AM · Restricted Project
ctetreau added inline comments to D94069: [NFC][InstructionCost]Migrate VectorCombine.cpp to use InstructionCost.
Mon, Jan 11, 9:38 AM · Restricted Project
ctetreau added a comment to D94069: [NFC][InstructionCost]Migrate VectorCombine.cpp to use InstructionCost.

Also, thank you for making clear that invalid, atm, means as well high cost.
I'll have that in mind for the next patches.

Mon, Jan 11, 9:23 AM · Restricted Project

Wed, Jan 6

ctetreau added inline comments to D94069: [NFC][InstructionCost]Migrate VectorCombine.cpp to use InstructionCost.
Wed, Jan 6, 9:49 AM · Restricted Project

Tue, Jan 5

ctetreau added a comment to D92238: [SCEVExpander] Migrate costAndCollectOperands to use InstructionCost..

I think this is conflating use-cases.
I'm not sure the entire InstructionCost should be used as a variable to track full/remaining cost over a several instructions.
Perhaps there should be a[nother] abstraction for that?

Tue, Jan 5, 11:00 AM · Restricted Project
ctetreau added a comment to D93639: [AArch64][SVE]Add cost model for vector reduce for scalable vector.

Have a couple nits, but seems reasonable to me. Somebody else should review as well.

Tue, Jan 5, 10:13 AM · Restricted Project
ctetreau added inline comments to D94065: [NFC] Make remaining cost functions in LoopVectorize.cpp use InstructionCost.
Tue, Jan 5, 9:38 AM · Restricted Project
ctetreau added inline comments to D94069: [NFC][InstructionCost]Migrate VectorCombine.cpp to use InstructionCost.
Tue, Jan 5, 9:30 AM · Restricted Project

Mon, Jan 4

ctetreau added a comment to rG91e66bfd321f: Revert "Use std::is_trivially_copyable", breaks MSVC build.

I know this is settled already, but I just wanted to confirm that I'm currently seeing this issue in my downstream. When building opt, line 254 of CodeGen/WasEHPrepare.cpp fails to compile:

Mon, Jan 4, 2:24 PM

Dec 10 2020

ctetreau added a comment to D91174: [Support] Introduce a new InstructionCost class.

Please add the asserts in SLPVectorizer, and then this looks good to me.

Dec 10 2020, 6:22 AM · Restricted Project
ctetreau added inline comments to D93030: [AArch64][SVE]Add cost model for masked gather and scatter for scalable vector..
Dec 10 2020, 6:09 AM · Restricted Project

Dec 9 2020

ctetreau added inline comments to D91174: [Support] Introduce a new InstructionCost class.
Dec 9 2020, 11:54 AM · Restricted Project
ctetreau added inline comments to D91174: [Support] Introduce a new InstructionCost class.
Dec 9 2020, 11:52 AM · Restricted Project

Dec 8 2020

ctetreau added a comment to D91174: [Support] Introduce a new InstructionCost class.

I think this is pretty much fine. Please add the increment and decrement tests and this looks good to me.

Dec 8 2020, 3:11 PM · Restricted Project

Dec 3 2020

ctetreau added a comment to D92094: [CostModel]Replace FixedVectorType by VectorType in costgetIntrinsicInstrCost.

Hi @ctetreau.
I don't know if you are asking tests for Scalable Vector or FixedVectorType.

Dec 3 2020, 7:18 AM · Restricted Project
ctetreau added inline comments to D91174: [Support] Introduce a new InstructionCost class.
Dec 3 2020, 5:40 AM · Restricted Project
ctetreau added a comment to D92094: [CostModel]Replace FixedVectorType by VectorType in costgetIntrinsicInstrCost.

I see a bunch of new intrinsics being evaluated, but only tests for CTLZ and CCTZ. Do the others have coverage somewhere already?

Dec 3 2020, 5:35 AM · Restricted Project
ctetreau added a comment to D92094: [CostModel]Replace FixedVectorType by VectorType in costgetIntrinsicInstrCost.

... Yes we should do that. From having seen how this work is layered, it will be quite a substantial task ...

Dec 3 2020, 5:31 AM · Restricted Project
ctetreau added inline comments to D92178: [NFC][InstructionCost] Change LoopVectorizationCostModel::getInstructionCost to return InstructionCost.
Dec 3 2020, 5:23 AM · Restricted Project

Dec 1 2020

ctetreau added inline comments to D91174: [Support] Introduce a new InstructionCost class.
Dec 1 2020, 8:43 AM · Restricted Project
ctetreau added inline comments to D92178: [NFC][InstructionCost] Change LoopVectorizationCostModel::getInstructionCost to return InstructionCost.
Dec 1 2020, 8:39 AM · Restricted Project
ctetreau added a comment to D90342: [POC][LoopVectorizer] Propagate ElementCount to interfaces in preparation for scalable auto-vec. .

It looks like all the split-out patches have been merged. Is there anything in this patch that is still outstanding?

Dec 1 2020, 4:28 AM · Restricted Project

Nov 30 2020

ctetreau added a comment to D92177: [NFC][InstructionCost] Refactor LoopVectorizationCostModel::selectVectorizationFactor.

I don't think you needs these pairs. If you check validity of the InstructionCost in the for loop, and break or continue if it's invalid, then you can just use a scalar value as it did before. (I commented as such on D92178)

Nov 30 2020, 9:58 AM · Restricted Project
ctetreau added inline comments to D92178: [NFC][InstructionCost] Change LoopVectorizationCostModel::getInstructionCost to return InstructionCost.
Nov 30 2020, 9:56 AM · Restricted Project
ctetreau added inline comments to D92177: [NFC][InstructionCost] Refactor LoopVectorizationCostModel::selectVectorizationFactor.
Nov 30 2020, 7:32 AM · Restricted Project
ctetreau added a comment to D92094: [CostModel]Replace FixedVectorType by VectorType in costgetIntrinsicInstrCost.

I'd prefer if we didn't knowingly create broken paths here

Nov 30 2020, 7:18 AM · Restricted Project
ctetreau added a comment to D91174: [Support] Introduce a new InstructionCost class.

I personally feel very strongly that we should define a total ordering here as is currently being done. @nhaehnle says that this behaves like nan, and that this is bad, and that it runs the risk of users relying on this total ordering. I disagree with all three of these concerns.

Nov 30 2020, 5:40 AM · Restricted Project

Nov 24 2020

ctetreau accepted D91174: [Support] Introduce a new InstructionCost class.

I'll go ahead an approve the patch for now to clear the "red x" next to my name. Please add the test before merging though.

Nov 24 2020, 9:02 AM · Restricted Project
ctetreau added a comment to D91174: [Support] Introduce a new InstructionCost class.

I don't know if having that red x next to my name causes phabricator to be difficult. If so, I apologize.

Nov 24 2020, 9:00 AM · Restricted Project
ctetreau added a comment to D91174: [Support] Introduce a new InstructionCost class.

I marked it "needs changes" to rescind my acceptance due to Sander's issue. However, I'm fine with this (aside from the test I requested), so feel free to merge it once the test is added and Sander is satisfied.

Nov 24 2020, 9:00 AM · Restricted Project

Nov 23 2020

ctetreau added a comment to D91174: [Support] Introduce a new InstructionCost class.

I suppose I'm fine with Optional<CostType> for the purpose of future-proofing. Users should be doing arithmetic on InstructionCost anyways, so there shouldn't be a ton of conversions. Maybe we can just provide operator* instead of making the user write *(C.getCost()).

Nov 23 2020, 8:50 AM · Restricted Project
ctetreau added a comment to D91174: [Support] Introduce a new InstructionCost class.

I'm likely to be out of the office most of this week, so once the assert is in, feel free to merge without waiting for my approval (assuming Sander is satisfied)

Nov 23 2020, 8:47 AM · Restricted Project
ctetreau requested changes to D91174: [Support] Introduce a new InstructionCost class.
Nov 23 2020, 8:46 AM · Restricted Project

Nov 20 2020

ctetreau added a comment to D91727: [LAA] NFC: Rename [get]MaxSafeRegisterWidth -> [get]MaxSafeVectorWidthInBits.

Please run clang-format, then this looks good to me.

Nov 20 2020, 3:18 PM · Restricted Project
ctetreau added a comment to D90344: [POC][LoopVectorizer] Allow invariant loads/stores using masked gather/scatter for a scalable VF..

I have no objections to this, but you should probably get some more eyes on it.

Nov 20 2020, 9:50 AM · Restricted Project
ctetreau accepted D91174: [Support] Introduce a new InstructionCost class.

Looks good to me

Nov 20 2020, 9:46 AM · Restricted Project

Nov 19 2020

ctetreau added a comment to D91174: [Support] Introduce a new InstructionCost class.

This is pretty much there, just a few nits and it will be good to go.

Nov 19 2020, 4:45 PM · Restricted Project
ctetreau added inline comments to D91727: [LAA] NFC: Rename [get]MaxSafeRegisterWidth -> [get]MaxSafeVectorWidthInBits.
Nov 19 2020, 4:28 PM · Restricted Project

Nov 17 2020

ctetreau committed rG792f8e1114af: [SVE] Take constant fold fast path for splatted vscale vectors (authored by ctetreau).
[SVE] Take constant fold fast path for splatted vscale vectors
Nov 17 2020, 12:46 PM
ctetreau closed D77442: [SVE] Take constant fold fast path for splatted vscale vectors.
Nov 17 2020, 12:45 PM · Restricted Project

Nov 16 2020

ctetreau updated the diff for D77442: [SVE] Take constant fold fast path for splatted vscale vectors.

Fix broken test

Nov 16 2020, 3:09 PM · Restricted Project
ctetreau added a comment to D77442: [SVE] Take constant fold fast path for splatted vscale vectors.

Ping?

Nov 16 2020, 1:48 PM · Restricted Project
ctetreau updated the diff for D77442: [SVE] Take constant fold fast path for splatted vscale vectors.

Necro patch. address code review issues.

Nov 16 2020, 1:45 PM · Restricted Project
ctetreau abandoned D91564: [SVE] Take constant fold fast path for splatted vscale vectors.
Nov 16 2020, 1:45 PM · Restricted Project
ctetreau requested review of D91564: [SVE] Take constant fold fast path for splatted vscale vectors.
Nov 16 2020, 1:42 PM · Restricted Project

Nov 13 2020

ctetreau added inline comments to D91174: [Support] Introduce a new InstructionCost class.
Nov 13 2020, 10:20 AM · Restricted Project

Nov 10 2020

ctetreau added inline comments to D91174: [Support] Introduce a new InstructionCost class.
Nov 10 2020, 10:11 AM · Restricted Project
ctetreau added a comment to D91174: [Support] Introduce a new InstructionCost class.

Overall, this seems reasonable to me.

Nov 10 2020, 10:09 AM · Restricted Project

Nov 9 2020

ctetreau accepted D90880: [LoopVectorizer] NFC: Return ElementCount from compute[Feasible]MaxVF.

looks good to me

Nov 9 2020, 9:38 AM · Restricted Project
ctetreau accepted D90879: [LoopVectorizer] NFC: Propagate ElementCount to more interfaces..

looks good to me

Nov 9 2020, 9:31 AM · Restricted Project

Nov 6 2020

ctetreau added inline comments to D90761: [docs] Adding a Support Policy.
Nov 6 2020, 4:00 PM · Restricted Project

Nov 5 2020

ctetreau added a reviewer for D90344: [POC][LoopVectorizer] Allow invariant loads/stores using masked gather/scatter for a scalable VF.: ctetreau.
Nov 5 2020, 2:22 PM · Restricted Project
ctetreau added inline comments to D90344: [POC][LoopVectorizer] Allow invariant loads/stores using masked gather/scatter for a scalable VF..
Nov 5 2020, 2:21 PM · Restricted Project
ctetreau added inline comments to D90342: [POC][LoopVectorizer] Propagate ElementCount to interfaces in preparation for scalable auto-vec. .
Nov 5 2020, 2:16 PM · Restricted Project
ctetreau accepted D90713: [TypeSize] Extend UnivariateLinearPolyBase with getWithIncrement/Decrement methods.

looks good to me

Nov 5 2020, 2:09 PM · Restricted Project
ctetreau added inline comments to D90761: [docs] Adding a Support Policy.
Nov 5 2020, 9:33 AM · Restricted Project

Nov 4 2020

ctetreau added a comment to D90761: [docs] Adding a Support Policy.

I've suggested some changes, but this seems reasonable to me.

Nov 4 2020, 11:55 AM · Restricted Project
ctetreau committed rG900ec97bbe32: [UBSan] Cannot negate smallest negative signed integer (authored by ctetreau).
[UBSan] Cannot negate smallest negative signed integer
Nov 4 2020, 10:08 AM
ctetreau closed D90710: [UBSan] Cannot negate smallest negative signed integer.
Nov 4 2020, 10:08 AM · Restricted Project

Nov 3 2020

ctetreau added reviewers for D90710: [UBSan] Cannot negate smallest negative signed integer: paulwalker-arm, kmclaughlin, cameron.mcinally, c-rhodes, efriedma.
Nov 3 2020, 1:45 PM · Restricted Project
ctetreau requested review of D90710: [UBSan] Cannot negate smallest negative signed integer.
Nov 3 2020, 12:59 PM · Restricted Project

Oct 30 2020

ctetreau accepted D88982: [NFCI] Add StackOffset class and base classes for ElementCount, TypeSize..

Yeah, I think this is good to go

Oct 30 2020, 12:57 PM · Restricted Project, Restricted Project

Oct 29 2020

ctetreau added a reviewer for D90342: [POC][LoopVectorizer] Propagate ElementCount to interfaces in preparation for scalable auto-vec. : ctetreau.
Oct 29 2020, 1:41 PM · Restricted Project
ctetreau added a comment to D90342: [POC][LoopVectorizer] Propagate ElementCount to interfaces in preparation for scalable auto-vec. .

This seems reasonable to me.

Oct 29 2020, 1:41 PM · Restricted Project

Oct 28 2020

ctetreau added a comment to D88982: [NFCI] Add StackOffset class and base classes for ElementCount, TypeSize..

Looks good to me once you add that assert.

Oct 28 2020, 9:50 AM · Restricted Project, Restricted Project

Oct 27 2020

ctetreau abandoned D89818: [Compile Time] Make it possible to skip redundant debuginst removal.

The other patch was merged.

Oct 27 2020, 11:55 AM · Restricted Project
ctetreau accepted D88928: [Utils] Skip RemoveRedundantDbgInstrs in MergeBlockIntoPredecessor (PR47746).

Looks good to me.

Oct 27 2020, 9:40 AM · Restricted Project

Oct 26 2020

ctetreau added inline comments to D88982: [NFCI] Add StackOffset class and base classes for ElementCount, TypeSize..
Oct 26 2020, 4:17 PM · Restricted Project, Restricted Project
ctetreau added inline comments to D88982: [NFCI] Add StackOffset class and base classes for ElementCount, TypeSize..
Oct 26 2020, 1:00 PM · Restricted Project, Restricted Project
ctetreau added inline comments to D88982: [NFCI] Add StackOffset class and base classes for ElementCount, TypeSize..
Oct 26 2020, 12:04 PM · Restricted Project, Restricted Project

Oct 22 2020

ctetreau added inline comments to D88982: [NFCI] Add StackOffset class and base classes for ElementCount, TypeSize..
Oct 22 2020, 2:46 PM · Restricted Project, Restricted Project

Oct 21 2020

ctetreau added a comment to D88928: [Utils] Skip RemoveRedundantDbgInstrs in MergeBlockIntoPredecessor (PR47746).
In D88928#2343175, @vsk wrote:

I was hoping for another +1 after Jeremy lgtm'd this change to be safe. Seeing as this appears to be time-sensitive, if there aren't any concerns/objections I'll land this by end of day tomorrow (PST).

Oct 21 2020, 1:54 PM · Restricted Project
ctetreau added inline comments to D88982: [NFCI] Add StackOffset class and base classes for ElementCount, TypeSize..
Oct 21 2020, 10:16 AM · Restricted Project, Restricted Project
ctetreau committed rG26459e6d8eeb: Fix "Unknown arguments specified" to if in lldb (authored by ctetreau).
Fix "Unknown arguments specified" to if in lldb
Oct 21 2020, 7:25 AM
ctetreau closed D89807: Fix "Unknown arguments specified" to if in lldb.
Oct 21 2020, 7:25 AM · Restricted Project

Oct 20 2020

ctetreau accepted D89531: [SVE] Remove reliance on TypeSize comparison operators in unit tests.

looks good to me

Oct 20 2020, 5:11 PM · Restricted Project
ctetreau added a comment to D89818: [Compile Time] Make it possible to skip redundant debuginst removal.

I think your patch is reasonable. Mine is more conservative, yours more aggressive. Both solve the problem. If this patch is merged, yours should be abandoned.

Oct 20 2020, 3:51 PM · Restricted Project
ctetreau added a comment to D88928: [Utils] Skip RemoveRedundantDbgInstrs in MergeBlockIntoPredecessor (PR47746).

I don't know if this patch as-is exhaustively removes redundant debug instructions in all cases it did before, or if we care, so I can't approve it, but I am OK with this approach.

Oct 20 2020, 3:50 PM · Restricted Project
ctetreau added a comment to D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE.

I would assume so. I'll take a look.

Oct 20 2020, 3:45 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
ctetreau added a comment to D89807: Fix "Unknown arguments specified" to if in lldb.

OK, for now, I've restored the use of uppercase_CMAKE_BUILD_TYPE. It looks like this does get set, and it's just the empty string on my machine because windows.

Oct 20 2020, 3:25 PM · Restricted Project
ctetreau updated the diff for D89807: Fix "Unknown arguments specified" to if in lldb.

restore use of uppercase_CMAKE_BUILD_TYPE

Oct 20 2020, 3:22 PM · Restricted Project
ctetreau committed rG2eac8ce820e6: Get the address space within getVectorPtrTy (authored by ctetreau).
Get the address space within getVectorPtrTy
Oct 20 2020, 2:42 PM
ctetreau closed D89745: Get the address space within getVectorPtrTy.
Oct 20 2020, 2:42 PM · Restricted Project
ctetreau updated the diff for D89818: [Compile Time] Make it possible to skip redundant debuginst removal.

address code review issues

Oct 20 2020, 2:05 PM · Restricted Project
ctetreau added a comment to D89818: [Compile Time] Make it possible to skip redundant debuginst removal.

Is this an NFC patch? If not, can you please add a test case?

Oct 20 2020, 2:03 PM · Restricted Project
ctetreau added a comment to D88928: [Utils] Skip RemoveRedundantDbgInstrs in MergeBlockIntoPredecessor (PR47746).

I pushed D89818, which is based on the patch that I submitted to my downstream to fix this issue.

Oct 20 2020, 1:01 PM · Restricted Project
ctetreau added a comment to D89818: [Compile Time] Make it possible to skip redundant debuginst removal.

This patch is basically the same thing we ended up doing in our downstream to resolve the issue. We're seeing it in LoopUnroll, so that's where we are skipping it.

Oct 20 2020, 12:59 PM · Restricted Project
ctetreau added reviewers for D89818: [Compile Time] Make it possible to skip redundant debuginst removal: fhahn, bjope, aprantl, jmorse, efriedma.
Oct 20 2020, 12:57 PM · Restricted Project
ctetreau requested review of D89818: [Compile Time] Make it possible to skip redundant debuginst removal.
Oct 20 2020, 12:55 PM · Restricted Project
ctetreau added a comment to D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE.

I would assume so. I'll take a look.

Oct 20 2020, 12:37 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
ctetreau abandoned D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE.
Oct 20 2020, 12:15 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
ctetreau added a comment to D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE.

I quite like this. We should strive to use the same name as CMake uses for its build types, i.e. Debug, Release, etc.

If I configure CMake with -DCMAKE_BUILD_TYPE=DEBUG, is CMAKE_BUILD_TYPE normalized back to Debug by CMake, or is it defined to DEBUG? If the latter, then I suggest we might want to add a temporary error message when the CMAKE_BUILD_TYPE is defined to something all-uppercase. This would help catch cases where this patch will be a change in behavior. That would be a nice place to tell users to use the right build type names.

Oct 20 2020, 12:15 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
ctetreau planned changes to D89807: Fix "Unknown arguments specified" to if in lldb.

After experimenting a bit, it seems that if you pass -DCMAKE_BUILD_TYPE=RELEASE, then the value of CMAKE_BUILD_TYPE will also be uppercase. This might represent a "good reason" to use the uppercase_CMAKE_BUILD_TYPE version. I need to think on this a bit, but I may end up just making sure that this variable is defined.

Oct 20 2020, 11:59 AM · Restricted Project
ctetreau requested review of D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE.
Oct 20 2020, 11:29 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
ctetreau added a comment to D89807: Fix "Unknown arguments specified" to if in lldb.

What's the motivation for this?

Oct 20 2020, 11:04 AM · Restricted Project
ctetreau added a reviewer for D89807: Fix "Unknown arguments specified" to if in lldb: efriedma.
Oct 20 2020, 10:11 AM · Restricted Project