qcolombet (Quentin Colombet)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 17 2012, 10:03 AM (248 w, 6 d)

Recent Activity

Thu, Sep 21

qcolombet added a comment to D38128: Handle COPYs of physregs better.

calculateSpillWeightAndHint, why is this not working here?

Thu, Sep 21, 10:28 AM

Wed, Sep 20

qcolombet committed rL313774: [InstCombine] Add select simplifications.
[InstCombine] Add select simplifications
Wed, Sep 20, 10:34 AM
qcolombet closed D37019: Add select simplifications by committing rL313774: [InstCombine] Add select simplifications.
Wed, Sep 20, 10:34 AM

Tue, Sep 19

qcolombet committed rL313696: [MIRPrinter] Print empty successor lists when they cannot be guessed.
[MIRPrinter] Print empty successor lists when they cannot be guessed
Tue, Sep 19, 4:36 PM
qcolombet committed rL313686: Revert "[MIRPrinter] Print empty successor lists when they cannot be guessed".
Revert "[MIRPrinter] Print empty successor lists when they cannot be guessed"
Tue, Sep 19, 3:05 PM
qcolombet committed rL313685: [MIRPrinter] Print empty successor lists when they cannot be guessed.
[MIRPrinter] Print empty successor lists when they cannot be guessed
Tue, Sep 19, 2:57 PM

Mon, Sep 18

qcolombet accepted D37775: Add a verifier test to check the access on both sides of COPY are the same.
Mon, Sep 18, 10:51 AM
qcolombet added a comment to D35816: [Greedy RegAlloc] Add logic to greedy reg alloc to avoid bad eviction chains.

For the record, playing with the order helps but to do the optimal (local) coloring here I would need to spend more time on the heuristic.
Again the easiest fix is probably the hints reconciling by region.

Mon, Sep 18, 10:46 AM
qcolombet added a comment to D37019: Add select simplifications.

@majnemer
Hi David,

Mon, Sep 18, 9:50 AM

Fri, Sep 15

qcolombet accepted D37775: Add a verifier test to check the access on both sides of COPY are the same.

LGTM with one suggestion

Fri, Sep 15, 2:28 PM

Thu, Sep 14

qcolombet added a comment to D35816: [Greedy RegAlloc] Add logic to greedy reg alloc to avoid bad eviction chains.

Hi Marina,

Thu, Sep 14, 5:29 PM

Wed, Sep 13

qcolombet added inline comments to D37640: [GISel]: Fix generation of illegal COPYs (of different sizes) during CallLowering .
Wed, Sep 13, 2:56 PM
qcolombet added a comment to D37640: [GISel]: Fix generation of illegal COPYs (of different sizes) during CallLowering .

The AArch64 part looks good to me.

Wed, Sep 13, 11:22 AM
qcolombet added inline comments to D37775: Add a verifier test to check the access on both sides of COPY are the same.
Wed, Sep 13, 10:42 AM
qcolombet added inline comments to D37775: Add a verifier test to check the access on both sides of COPY are the same.
Wed, Sep 13, 9:36 AM

Tue, Sep 12

qcolombet accepted D37019: Add select simplifications.

Thanks Michael.

Tue, Sep 12, 2:40 PM

Mon, Sep 11

qcolombet added inline comments to D37019: Add select simplifications.
Mon, Sep 11, 4:03 PM
qcolombet added a comment to D31834: Remove unnecessary bitvector clear.

Sorry, that one slipped through!

Mon, Sep 11, 4:00 PM
qcolombet added inline comments to D37019: Add select simplifications.
Mon, Sep 11, 3:50 PM
qcolombet added inline comments to D37019: Add select simplifications.
Mon, Sep 11, 3:46 PM
qcolombet added a comment to D37640: [GISel]: Fix generation of illegal COPYs (of different sizes) during CallLowering .

Hi Aditya,

Mon, Sep 11, 12:15 PM
qcolombet accepted D36795: [SystemZ] Increase number of LOCRs emitted by passing regalloc hints.

Do you think that looking at the number of spilled live ranges is a good way to consider the trade-off for using hard hints, or do you perhaps have anything to add?

Mon, Sep 11, 9:53 AM

Fri, Sep 8

qcolombet added a comment to D36795: [SystemZ] Increase number of LOCRs emitted by passing regalloc hints.

The generic change looks fine though it sounds surprising that anyone would want that.

Fri, Sep 8, 12:48 PM
qcolombet accepted D37578: [RegAlloc] Keep a copy of live interval for the spilled vregs in HoistSpillHelper.

LGTM with a test case (.mir preferably) from PR34502

Fri, Sep 8, 11:04 AM

Aug 25 2017

qcolombet accepted D36913: TableGen: Fix subreg composition/concatenation.

LGTM.
Nitpicks below, no need to submit a new version.

Aug 25 2017, 10:42 AM
qcolombet accepted D36911: TableGen: Add --gen-register-info-debug-dump.

LGTM too.
Same comment as Krzysztof, would be nice if the debug printing was more intuitively discoverable (e.g., -debug alongside with -gen-register-info).

Aug 25 2017, 10:19 AM

Aug 24 2017

qcolombet added inline comments to D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
Aug 24 2017, 5:02 PM
qcolombet accepted D37097: Set hasSideEffects=0 for PHI and fix passes relying isSafeToMove/hasUnmodeledSideEffects being true for PHI.

LGTM

Aug 24 2017, 4:59 PM
qcolombet accepted D37018: [GISel]: Legalize G_PHIs.

Few more nitpicks, no need for more reviews.

Aug 24 2017, 4:45 PM
qcolombet added a comment to D37018: [GISel]: Legalize G_PHIs.

I miss these test cases:

  • Add a test case with def feeding into two different phis:
    • In the same destination block
    • In different basic block (one on the true branch, one on the false branch)
Aug 24 2017, 2:27 PM
qcolombet added a comment to D37018: [GISel]: Legalize G_PHIs.

Hi Aditya,

Aug 24 2017, 12:41 PM
qcolombet added inline comments to D37097: Set hasSideEffects=0 for PHI and fix passes relying isSafeToMove/hasUnmodeledSideEffects being true for PHI.
Aug 24 2017, 10:47 AM

Aug 23 2017

qcolombet added inline comments to D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
Aug 23 2017, 2:59 PM
qcolombet accepted D36990: [GISel]: Translate phi into generic G_PHI instruction.

LGTM

Aug 23 2017, 11:07 AM
qcolombet added inline comments to D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
Aug 23 2017, 10:28 AM
qcolombet added a comment to D35816: [Greedy RegAlloc] Add logic to greedy reg alloc to avoid bad eviction chains.

Hi Marina,

Aug 23 2017, 10:24 AM
qcolombet added inline comments to D36990: [GISel]: Translate phi into generic G_PHI instruction.
Aug 23 2017, 10:20 AM
qcolombet added inline comments to D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
Aug 23 2017, 9:45 AM

Aug 22 2017

qcolombet added a comment to D37016: [GISel]: Allow the MachineIRBuilder to insert instructions after.

Good point. I think this is not needed. I'll update the other patch to use the std::next version. We can add this back at a later time if it's of value.

You can probably directly use setInsertPt with getFirstTerminator I think (i.e., no need to use std::next ;))

Aug 22 2017, 4:45 PM
qcolombet added a comment to D37016: [GISel]: Allow the MachineIRBuilder to insert instructions after.

Note: Given the other review, I let you decide if we need that patch or not.

Aug 22 2017, 4:33 PM
qcolombet added a comment to D37018: [GISel]: Legalize G_PHIs.

Hi Aditya,

Aug 22 2017, 4:31 PM
qcolombet added a comment to D36990: [GISel]: Translate phi into generic G_PHI instruction.

Hi Aditya,

Aug 22 2017, 4:17 PM
qcolombet added a comment to D37016: [GISel]: Allow the MachineIRBuilder to insert instructions after.

Just one nit.

Aug 22 2017, 4:13 PM
qcolombet accepted D37016: [GISel]: Allow the MachineIRBuilder to insert instructions after.

I believe we use to do that by using std::next([Instr | Iterator]WeWantToInsertAfter), but I can see how that could make things easier, so LGTM

Aug 22 2017, 4:09 PM

Aug 21 2017

qcolombet committed rL311401: [RegAlloc] Make sure live-ranges reflect the state of the IR when removing them.
[RegAlloc] Make sure live-ranges reflect the state of the IR when removing them
Aug 21 2017, 3:57 PM

Aug 15 2017

qcolombet committed rL310979: [VirtRegRewriter] Properly model the register liveness on undef subreg….
[VirtRegRewriter] Properly model the register liveness on undef subreg…
Aug 15 2017, 5:18 PM
qcolombet committed rL310969: Reapply "[GlobalISel] Remove the GISelAccessor API.".
Reapply "[GlobalISel] Remove the GISelAccessor API."
Aug 15 2017, 3:32 PM

Aug 14 2017

qcolombet added a comment to D35816: [Greedy RegAlloc] Add logic to greedy reg alloc to avoid bad eviction chains.

Based on this I think the solution should probably be kept in Greedy (+ possibly additional cleanup in the copy propagation pass).

Aug 14 2017, 1:37 PM
qcolombet accepted D36698: IPRA: Allow target to enable IPRA by default.

LGTM with one nit

Aug 14 2017, 11:37 AM
qcolombet accepted D36695: IPRA: Run RegUsageInfoPropagate much later.

LGTM

Aug 14 2017, 9:54 AM
qcolombet accepted D36636: [GISel][NFC]: Add some constructors for easy creation of MIRBuilders.

LGTM.
One nit below.

Aug 14 2017, 9:29 AM
qcolombet accepted D30751: [MachineCopyPropagation] Extend pass to do COPY source forwarding.

Thanks for your patience Geoff.

Aug 14 2017, 9:28 AM
qcolombet added inline comments to D36636: [GISel][NFC]: Add some constructors for easy creation of MIRBuilders.
Aug 14 2017, 9:19 AM
qcolombet added inline comments to D36636: [GISel][NFC]: Add some constructors for easy creation of MIRBuilders.
Aug 14 2017, 9:12 AM

Aug 9 2017

qcolombet added inline comments to D35014: [X86] PR32755 : Improvement in CodeGen instruction selection for LEAs..
Aug 9 2017, 10:59 AM

Aug 8 2017

qcolombet committed rL310425: Revert "[GlobalISel] Remove the GISelAccessor API.".
Revert "[GlobalISel] Remove the GISelAccessor API."
Aug 8 2017, 3:23 PM

Aug 7 2017

qcolombet added inline comments to D33814: CodeGen: Fix ARM cmpxchg64 register fragmentation in fast-regalloc.
Aug 7 2017, 10:37 AM
qcolombet requested changes to D33814: CodeGen: Fix ARM cmpxchg64 register fragmentation in fast-regalloc.

The approach sounds sensible to me however, I'd like we use the AllocationPriority instead of the size of the register class.
Moreover, please refactor the code (I am thinking helper function) so that the definitions are processed in the same way.
Finally, keep the order of the operand as tie breaker.

Aug 7 2017, 10:37 AM

Aug 4 2017

qcolombet committed rL310115: [GlobalISel] Remove the GISelAccessor API..
[GlobalISel] Remove the GISelAccessor API.
Aug 4 2017, 1:16 PM
qcolombet committed rL310114: [GlobalISel] Remove a stall comment in CMake..
[GlobalISel] Remove a stall comment in CMake.
Aug 4 2017, 1:16 PM
qcolombet accepted D36301: Adding ChangeToTargetIndex function to MachineOperand.

LGTM

Aug 4 2017, 9:11 AM

Aug 3 2017

qcolombet committed rL309990: [GlobalISel] Make GlobalISel a non-optional library..
[GlobalISel] Make GlobalISel a non-optional library.
Aug 3 2017, 2:53 PM
qcolombet requested changes to D30751: [MachineCopyPropagation] Extend pass to do COPY source forwarding.
Aug 3 2017, 2:47 PM
qcolombet accepted D36170: LSR: Fix PR33514.

Thanks for the explanation.

Aug 3 2017, 12:58 PM

Aug 2 2017

qcolombet accepted D36204: [RegisterCoalescer] Add wrapper for Erasing Instructions.

LGTM

Aug 2 2017, 6:51 PM
qcolombet added a comment to D36170: LSR: Fix PR33514.

If we insert more converts we should raise Formula cost somehow (most likely leading to the formula drop).

Aug 2 2017, 2:16 PM

Aug 1 2017

qcolombet added a comment to D36160: Add "Restored" flag to CalleeSavedInfo.

My guts say we should make the frame lowering logic more complex than it is already.

*shouldn't

Aug 1 2017, 1:31 PM
qcolombet added a comment to D36170: LSR: Fix PR33514.

Hi Evgeny,

Aug 1 2017, 1:29 PM
qcolombet added a comment to D36160: Add "Restored" flag to CalleeSavedInfo.

Long-term, it might be more straightforward to add implicit uses to return instructions, similar to the way we add implicit defs to calls, so we don't have to make return blocks a special case in LivePhysRegs. Not sure how hard that would be.

Aug 1 2017, 12:30 PM

Jul 31 2017

qcolombet accepted D35933: Eliminate TargetTransformInfo::isFoldableMemAccess().

Hi Jonas,

Jul 31 2017, 4:41 PM
qcolombet added a comment to D35594: [GISel]: ConstantFold operations when building MIR.

Is that one flag that basically enables/disables all optimizations/folding ?

Yes, I would go with that for now. But I could see your concerns that we are going to generate crapy code for no good reason, thus you'd like more control.

Jul 31 2017, 2:04 PM
qcolombet committed rL309600: [llc][NFC] Update message in assert..
[llc][NFC] Update message in assert.
Jul 31 2017, 11:32 AM
qcolombet committed rL309599: [TargetPassConfig] Feature generic options to setup start/stop-after/before.
[TargetPassConfig] Feature generic options to setup start/stop-after/before
Jul 31 2017, 11:25 AM
qcolombet closed D30913: [NFC] Feature generic options to setup start/stop-after/before by committing rL309599: [TargetPassConfig] Feature generic options to setup start/stop-after/before.
Jul 31 2017, 11:24 AM
qcolombet added inline comments to D30913: [NFC] Feature generic options to setup start/stop-after/before.
Jul 31 2017, 10:09 AM

Jul 28 2017

qcolombet added a comment to D35594: [GISel]: ConstantFold operations when building MIR.

Disabled constant folding cast instructions as MIR currently doesn't infer legality of constants of type that it creates.

Jul 28 2017, 3:24 PM
qcolombet added a reviewer for D35907: [StackColoring] Update AliasAnalysis information in stack coloring pass: hfinkel.

I think Hal would like to double check that.

Jul 28 2017, 3:17 PM
qcolombet added a comment to D35907: [StackColoring] Update AliasAnalysis information in stack coloring pass.

Haha, never mind me, Hal already replied. (Sloppy tab!)

Jul 28 2017, 3:17 PM

Jul 26 2017

qcolombet updated the diff for D30913: [NFC] Feature generic options to setup start/stop-after/before.
  • Move all the logic to handle the options in TargetPassConfig
  • Refactor llc code to avoid code duplication, now only the run-pass pipeline is custom made
  • Add MMI output parameter to be able to parse the module and populate the MachineModuleInfo after creating the pipeline
Jul 26 2017, 10:31 PM

Jul 24 2017

qcolombet requested changes to D35816: [Greedy RegAlloc] Add logic to greedy reg alloc to avoid bad eviction chains.

The greedy allocator is already very complicated and I am not sure the additional complexity of the eviction track is worth it.
Is it something that could be cleaned up in machine copy propagation? The problem is very local so that sounds doable.

Jul 24 2017, 3:44 PM
qcolombet accepted D35805: RA: Replace asserts related to empty live intervals.

Hi Matt,

Jul 24 2017, 10:13 AM

Jul 21 2017

qcolombet added inline comments to D35730: RA: Remove assert on empty live intervals.
Jul 21 2017, 6:37 PM
qcolombet added inline comments to D35730: RA: Remove assert on empty live intervals.
Jul 21 2017, 6:09 PM
qcolombet added inline comments to D35730: RA: Remove assert on empty live intervals.
Jul 21 2017, 6:06 PM
qcolombet added inline comments to D35730: RA: Remove assert on empty live intervals.
Jul 21 2017, 6:00 PM
qcolombet added inline comments to D35730: RA: Remove assert on empty live intervals.
Jul 21 2017, 5:54 PM
qcolombet added a comment to D35749: RA: Remove another assert on empty intervals.

Before removing all those assertions, could we talk that through first?
We started a conversion in D35730.

Jul 21 2017, 5:54 PM
qcolombet added inline comments to D35730: RA: Remove assert on empty live intervals.
Jul 21 2017, 5:38 PM
qcolombet added inline comments to D30751: [MachineCopyPropagation] Extend pass to do COPY source forwarding.
Jul 21 2017, 2:48 PM
qcolombet added inline comments to D35730: RA: Remove assert on empty live intervals.
Jul 21 2017, 1:51 PM

Jul 20 2017

qcolombet added a comment to D35049: LSR tunings for SystemZ, with some minor common code changes.

BTW, my review does not include SystemZ changes ;)

Jul 20 2017, 11:43 AM
qcolombet accepted D35049: LSR tunings for SystemZ, with some minor common code changes.

Hi Jonas,

Jul 20 2017, 11:43 AM
qcolombet accepted D35446: Add an ID field to StackObjects.

LGTM

Jul 20 2017, 11:29 AM

Jul 14 2017

qcolombet added a comment to D30913: [NFC] Feature generic options to setup start/stop-after/before.

Hi Matthias,

Jul 14 2017, 3:57 PM
qcolombet requested changes to D33935: Allow rematerialization of ARM Thumb MOVi8 instruction in some contexts.

Hi Philip,

Jul 14 2017, 2:31 PM

Jul 12 2017

qcolombet added inline comments to D35049: LSR tunings for SystemZ, with some minor common code changes.
Jul 12 2017, 10:55 AM

Jul 11 2017

qcolombet added a comment to D35049: LSR tunings for SystemZ, with some minor common code changes.

I see why second batch depends on this patch. I wonder if it should though.
Hence, do we rely guard that check with LSRWithInstrQueries?

Jul 11 2017, 10:48 AM
qcolombet accepted D35262: Teach isAddressUse() in LoopStrengthReduce.cpp about memory intrinsics.

LGTM

Jul 11 2017, 10:30 AM

Jul 10 2017

qcolombet requested changes to D35049: LSR tunings for SystemZ, with some minor common code changes.

Hi Jonas,

Jul 10 2017, 3:31 PM

Jul 9 2017

qcolombet committed rL307427: [RegAllocFast] Add the proper initialize method to use the .mir infrastructure.
[RegAllocFast] Add the proper initialize method to use the .mir infrastructure
Jul 9 2017, 6:17 AM