Page MenuHomePhabricator

qcolombet (Quentin Colombet)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 17 2012, 10:03 AM (312 w, 2 d)

Recent Activity

Fri, Dec 7

qcolombet added inline comments to D55403: [PHIElimination] Allow breaking loop backedges in certain cases..
Fri, Dec 7, 8:59 AM
qcolombet added a comment to D55301: RegAlloc: Allow targets to split register allocation.

I'm not sure I follow this. These aren't spilled with ordinary copies

Fri, Dec 7, 8:52 AM

Thu, Dec 6

qcolombet added a comment to D55301: RegAlloc: Allow targets to split register allocation.

Hi Matt,

Thu, Dec 6, 9:15 AM

Thu, Nov 29

qcolombet added a comment to D54899: [LoopStrengthReduce] ComplexityLimit as an option.

[...]
With this form, the first three loads can be base + immediate offset, while the last can perform the post increment to create %base.iv.next. What do you think?

Thu, Nov 29, 8:43 AM

Wed, Nov 28

qcolombet accepted D54899: [LoopStrengthReduce] ComplexityLimit as an option.

LGTM with @gilr's comments addressed.

Wed, Nov 28, 3:02 PM
qcolombet added a comment to D53629: [GlobalISel] Restrict G_MERGE_VALUES capability and replace with new opcodes.

I meant we should handle G_BUILD_VECTOR to avoid regressions as we are able to legalize the same function with G_MERGE_VALUES.

Wed, Nov 28, 2:58 PM

Tue, Nov 27

qcolombet added a comment to D53629: [GlobalISel] Restrict G_MERGE_VALUES capability and replace with new opcodes.

Hi Volkan,

Tue, Nov 27, 8:46 AM

Wed, Nov 14

qcolombet accepted D54218: [MachineScheduler] Bias physical register immediate assignments.

LGTM

Wed, Nov 14, 9:14 AM

Tue, Nov 13

qcolombet added a comment to D54218: [MachineScheduler] Bias physical register immediate assignments.

I see a few checks that weaken the check (e.g., -next replaced by regular checks).
Is this expected and why?

Tue, Nov 13, 11:53 AM

Nov 8 2018

qcolombet added a comment to D54218: [MachineScheduler] Bias physical register immediate assignments.

Looks reasonable to me.
Could of comments/nitpicks inlined.

Nov 8 2018, 9:12 AM

Nov 7 2018

qcolombet accepted D51861: [LSR] Combine unfolded offset into invariant register.

Hi Gil,

Nov 7 2018, 10:42 AM

Nov 5 2018

qcolombet added inline comments to D51861: [LSR] Combine unfolded offset into invariant register.
Nov 5 2018, 8:55 AM

Nov 2 2018

qcolombet added a comment to D51861: [LSR] Combine unfolded offset into invariant register.

Since testing for optimized or illegal code in these cases isn't an option in these cases, such a LIT could check that the incorrect code is not generated, that the incorrect formula is not generated, or that the correct formula is generated - which all seem fragile and overfitted to this specific bug, so I'm reluctant to add any of them (especially since the bug is covered by 4 runtime tests). What do you think?

Nov 2 2018, 10:08 AM
qcolombet added a comment to D53594: [GlobalISel] Introduce G_BUILD_VECTOR and G_CONCAT_VECTOR opcodes.

I think the issue of sequences becoming illegal because of things like copies being introduced is still a deal breaker for that, and it would also introduce a code motion barrier since we can't select sequences spanning blocks. E.g. it wouldn't be safe to move the insn in the predecessor due to other uses.

Nov 2 2018, 10:05 AM

Nov 1 2018

qcolombet added a comment to D51861: [LSR] Combine unfolded offset into invariant register.

The failing runtime tests do (a) and (b) but not (c).

Nov 1 2018, 10:11 AM

Oct 31 2018

qcolombet added a comment to D53594: [GlobalISel] Introduce G_BUILD_VECTOR and G_CONCAT_VECTOR opcodes.

I'm not sure how this is any better. If we had an opcode G_BUILD_VECTOR_TRUNC, it would have a superset of the functionality of the G_BUILD_VECTOR. You'd still have this powerful opcode, except now you have potential code duplication in handling the two, as well as a hindered G_BUILD_VECTOR.

Unless you're say that G_BUILD_VECTOR _TRUNC can *only* deal with oversize scalars, in which case it's an odd opcode for a corner case (and I would suggest here that we just deal with the inconvenient types, but I'm not married to the idea).

Oct 31 2018, 3:47 PM

Oct 30 2018

qcolombet committed rL345644: [InstCombine] Teach the move free before null test opti how to deal with noop….
[InstCombine] Teach the move free before null test opti how to deal with noop…
Oct 30 2018, 1:53 PM
qcolombet closed D53356: [InstCombine] Teach the move free before null test opti how to deal with noop casts.
Oct 30 2018, 1:53 PM
qcolombet added a comment to D53594: [GlobalISel] Introduce G_BUILD_VECTOR and G_CONCAT_VECTOR opcodes.

Thanks @aditya_nandakumar for the context.

Oct 30 2018, 11:48 AM
qcolombet added a comment to D53594: [GlobalISel] Introduce G_BUILD_VECTOR and G_CONCAT_VECTOR opcodes.
This allows implicit truncation.
Oct 30 2018, 10:29 AM
qcolombet added a comment to D51861: [LSR] Combine unfolded offset into invariant register.

Bug fix: ScalarEvolution::getAddExpr() may modify the vector of SCEVs it is given as argument, causing 2nd formula to be created with only part of the SCEVs in Ops.
This patch fixes this by using a temporary copy of Ops for the 1st formula.

Oct 30 2018, 10:27 AM

Oct 29 2018

qcolombet updated the diff for D53356: [InstCombine] Teach the move free before null test opti how to deal with noop casts.
  • Use m_CombineOr instead of repeating the pattern to match
Oct 29 2018, 9:49 AM
qcolombet added a comment to D53356: [InstCombine] Teach the move free before null test opti how to deal with noop casts.

Hi Sanjay,

Oct 29 2018, 9:43 AM

Oct 24 2018

qcolombet added a comment to D53356: [InstCombine] Teach the move free before null test opti how to deal with noop casts.

If it doesn't look too hard, moving it over to SimplifyCFG would be great. That way, we can be sure that the empty block removal always happens without interference from some other pass.

Oct 24 2018, 2:40 PM
qcolombet added a comment to D53356: [InstCombine] Teach the move free before null test opti how to deal with noop casts.

Hi Sanjay,

Oct 24 2018, 10:55 AM

Oct 23 2018

qcolombet added a comment to D53356: [InstCombine] Teach the move free before null test opti how to deal with noop casts.

Ping

Oct 23 2018, 1:37 PM
qcolombet added a comment to D51861: [LSR] Combine unfolded offset into invariant register.

Thanks for the detailed numbers!

Oct 23 2018, 1:35 PM

Oct 19 2018

qcolombet added a comment to D33935: Allow rematerialization of ARM Thumb MOVi8 instruction in some contexts.

However, I would vote for that to be introduced when the problem actually arises. It isn't so obvious to me that many more generalizations of rematerializability will be necessary and the path from enum to bitfield is easy.

What do you think?

Oct 19 2018, 11:01 AM

Oct 17 2018

qcolombet added a comment to D33935: Allow rematerialization of ARM Thumb MOVi8 instruction in some contexts.

Sorry for the delay, I haven’t had time to review the patch but I have one high level comment.
I am not sure the enum approach is desirable as it basically hard code the kind of constraints we can report and will grow very quickly if we want to extend it.
For instance, let say that on top of physreg defs we want to report virtual reg defs, now we would need “yes”, “no”, “yes_phys”, “yes_virt”, “yes_phys_virt” and the list grows with the cross product of everything we may want/need to report.

Oct 17 2018, 1:50 PM

Oct 16 2018

qcolombet created D53356: [InstCombine] Teach the move free before null test opti how to deal with noop casts.
Oct 16 2018, 9:56 PM

Oct 9 2018

qcolombet accepted D35481: Fix documentation of MachineInstr::getNumOperands.

LGTM

Oct 9 2018, 8:52 AM

Oct 8 2018

qcolombet accepted D51861: [LSR] Combine unfolded offset into invariant register.

One question, what is the impact on compile time?
Put differently are the heuristics to prune the search space able to still reduce the space to something manageable?

Oct 8 2018, 3:27 PM

Sep 17 2018

qcolombet added a comment to D52061: [RegisterCoalescer] Only look at main ranges in valuesIdentical/followCopyChain.

But I still believe that trying to spot the identical case in subranges is pointless because we have already proved that the values are identical in main ranges. It is impossible for the values to be not identical in subranges; it's just a bit tricky to fix up the code to demonstrate that.

Sep 17 2018, 9:30 AM
qcolombet added a comment to D52131: [GISel][NFC]: Make MachineIRBuilder fully stateless.

Hi Aditya,

Sep 17 2018, 9:18 AM
qcolombet added a comment to D52127: [MF][MBB]: Add the ability to register callbacks for removal and insertion of MachineInstrs.

Hi Aditya,

Sep 17 2018, 9:12 AM
qcolombet added a comment to D51849: [RegisterCoalescer] Avoid "Use not jointly dominated by defs" in removePartialRedundancy.

I finally tracked down why I couldn't get a mir test before: Compiling a ll file as usual, arriving at RegisterCoalescer, the numbering of the MBBs was not quite in order. Using -stop-before names the MBBs in the mir according to those out-of-order numbers, but reading the mir in again with -run-pass fails to preserve it. Instead it renumbers the MBBs in order. RegisterCoalescing processes MBBs with the same loop depth and critical edgeness and connectedness in numbering order, so the pass behaved differently.

Sep 17 2018, 9:11 AM
qcolombet added a comment to D52052: [RegAllocGreedy] avoid using physreg candidates that cannot be correctly spilled.

Could you elaborate on the exact constraints?

Sep 17 2018, 9:05 AM · Restricted Project

Sep 14 2018

qcolombet added a comment to D52061: [RegisterCoalescer] Only look at main ranges in valuesIdentical/followCopyChain.

The fix here is to only detect an "identical erase" in the main range.

Sep 14 2018, 9:25 AM
qcolombet added a comment to D52052: [RegAllocGreedy] avoid using physreg candidates that cannot be correctly spilled.

Could you elaborate on that part?

But it is not possible since can generate incorrect code in terms of exec mask.

Sep 14 2018, 9:23 AM · Restricted Project

Sep 13 2018

qcolombet added a comment to D51953: [GlobalISel] Add a new IR canonicalization pass.

FWIW, I am still not convince this is a good thing to match SDISel behavior at O0, but this is the least bad solution of doing that IMHO.

Sep 13 2018, 10:12 AM
qcolombet added a comment to D52010: RegAllocFast: Rewrite and improve.

Hi Matthias,

Sep 13 2018, 9:52 AM
qcolombet added inline comments to D51953: [GlobalISel] Add a new IR canonicalization pass.
Sep 13 2018, 9:46 AM
qcolombet accepted D51953: [GlobalISel] Add a new IR canonicalization pass.

Hi Amara,

Sep 13 2018, 9:46 AM

Sep 11 2018

qcolombet added a comment to D51953: [GlobalISel] Add a new IR canonicalization pass.

Hi Amara,

Sep 11 2018, 3:46 PM

Sep 10 2018

qcolombet added a comment to D51849: [RegisterCoalescer] Avoid "Use not jointly dominated by defs" in removePartialRedundancy.

Unfortunately I can only reproduce this in an ll test. Turning it into a mir test makes the problem go away.

Sep 10 2018, 8:40 AM
qcolombet added a comment to D51848: [RegisterCoalescer] Fixup "Fixed inconsistent followCopyChain with subreg".

Hi Tim,

Sep 10 2018, 8:38 AM
qcolombet added a comment to D51574: [LiveRangeCalc] Fixed findReachingDefs bug.

Hi Tim,

Sep 10 2018, 8:34 AM

Sep 5 2018

qcolombet added a comment to D51362: [GlobalISel][IRTranslator] Canonicalize G_ICMP to have constant operands last.

If we agree that an input canonicalization pass is needed then doing this there adds much less complexity than teaching tablegen that G_ICMPs are special (or somehow adding legalisation rules to tablegen?).

Sep 5 2018, 8:22 AM

Aug 31 2018

qcolombet added a comment to D51362: [GlobalISel][IRTranslator] Canonicalize G_ICMP to have constant operands last.

So overall I think we should have some form of IR canonicalization before translation for the majority of cases to be handled for -O0.

Aug 31 2018, 10:19 AM
qcolombet added a comment to D51362: [GlobalISel][IRTranslator] Canonicalize G_ICMP to have constant operands last.

It can't do that all the time, with these G_ICMPs for example it also has to swap the predicate (not that we even have imported patterns that could possibly match for AArch64).

Aug 31 2018, 10:19 AM

Aug 30 2018

qcolombet added a comment to D51362: [GlobalISel][IRTranslator] Canonicalize G_ICMP to have constant operands last.

Going back to a higher-level description for all that stuff, I wonder if ISel could already handle this.

Aug 30 2018, 10:06 AM
qcolombet added a comment to D51362: [GlobalISel][IRTranslator] Canonicalize G_ICMP to have constant operands last.

how would it decide in which order to put those vregs in the generated instructions w/o explicitly checking for constants?

Aug 30 2018, 9:09 AM
qcolombet added a comment to D51362: [GlobalISel][IRTranslator] Canonicalize G_ICMP to have constant operands last.

Does that mean that something after the last InstCombine produces icmp's with the non-canonical operand order? If so maybe that something needs to be chased down and fixed instead.

Aug 30 2018, 8:50 AM

Aug 29 2018

qcolombet added a comment to D51362: [GlobalISel][IRTranslator] Canonicalize G_ICMP to have constant operands last.

I agree with the motivation but I don’t think this is the right approach. I would prefer we run a canonicalization step even at O0 before GISel rather than duplicating this logic between IR and MI.
Jump tables are a different beast and have their place in here because they don’t really make sense to do on IR.

Aug 29 2018, 8:44 AM

Aug 28 2018

qcolombet added a comment to D51362: [GlobalISel][IRTranslator] Canonicalize G_ICMP to have constant operands last.

Hi Amara,

Aug 28 2018, 5:36 PM

Aug 13 2018

qcolombet accepted D49512: [CodeGenPrepare] Add BothExtension type to PromotedInsts.

Thanks for your patience.
LGTM.
One final nit below. Feel free to commit directly.

Aug 13 2018, 9:28 AM

Aug 8 2018

qcolombet added inline comments to D49512: [CodeGenPrepare] Add BothExtension type to PromotedInsts.
Aug 8 2018, 2:46 PM

Aug 6 2018

qcolombet added inline comments to D49512: [CodeGenPrepare] Add BothExtension type to PromotedInsts.
Aug 6 2018, 5:17 PM
qcolombet added a comment to D49512: [CodeGenPrepare] Add BothExtension type to PromotedInsts.

To be more precise, the latest promotion should be correct, but if we want the full information of multiple extension, the current patch is I believe wrong.
E.g., zext (sext i8 to i16) to i32, doesn't give you the same warranties on the bit pattern than what zext i8 to i32 gives, whereas we won't be able to see that.

Aug 6 2018, 5:15 PM
qcolombet added a comment to D49512: [CodeGenPrepare] Add BothExtension type to PromotedInsts.

To be more precise, the latest promotion should be correct, but if we want the full information of multiple extension, the current patch is I believe wrong.
E.g., zext (sext i8 to i16) to i32, doesn't give you the same warranties on the bit pattern than what zext i8 to i32 gives, whereas we won't be able to see that.

Aug 6 2018, 5:14 PM
qcolombet added a comment to D49512: [CodeGenPrepare] Add BothExtension type to PromotedInsts.

Shouldn't we keep only the latest type and kind of extension?

Aug 6 2018, 5:09 PM
qcolombet accepted D49913: Make TargetInstrInfo::isCopyInstr return true for regular COPY-instructions.

LGTM.
Nitpicks on the comments.

Aug 6 2018, 4:38 PM

Jul 24 2018

qcolombet accepted D49735: [RegisterBankInfo] Ignore InstrMappings that create impossible to repair operands.

Silly me for not catching this during the first review.

Jul 24 2018, 11:56 AM

Jul 20 2018

qcolombet added a comment to D35481: Fix documentation of MachineInstr::getNumOperands.

Good catch!

Jul 20 2018, 5:20 PM

Jun 18 2018

qcolombet added a comment to D47999: MIR YAML TracksRegLiveness default to true in yaml parser.

Either changing the default (trackLiveness == true by default) or printing it is fine.

Jun 18 2018, 8:35 AM

Jun 15 2018

qcolombet added a comment to D41098: [InlineSpiller] Fix a crash due to lack of forward progress from remat.

Are you okay with us solving this just for STATEPOINTs at the moment by essentially just disabling remats for STATEPOINT uses?

Jun 15 2018, 9:31 AM
qcolombet added a comment to D47999: MIR YAML TracksRegLiveness default to true in yaml parser.

Hi Puyan,

Jun 15 2018, 8:58 AM
qcolombet added a comment to D43542: [CodeGen][FastRegAlloc] Disable registers spilling for a naked function (PR28641).

Hi Konstantin,

Jun 15 2018, 8:57 AM

May 25 2018

qcolombet added a comment to D45156: [MachineVerifier] Verify the RegUsageInfo collected for the current function..

Do you agree we could drop this patch?

May 25 2018, 8:48 AM

May 24 2018

qcolombet accepted D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

With the test case fixed

May 24 2018, 11:24 AM
qcolombet added a comment to D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

Up to you for the logging. We can keep it.

May 24 2018, 11:21 AM

May 23 2018

qcolombet updated subscribers of D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

Doesn't the target already do this? At least there's a CoveredBySubRegs flag in the Register .td class ...

May 23 2018, 1:09 PM
qcolombet accepted D46809: [GlobalISel] NFCI, Getting GlobalISel ~5% faster.
May 23 2018, 12:49 PM

May 22 2018

qcolombet accepted D45204: [X86][MIPS][ARM] New machine instruction property 'isMoveReg'.
May 22 2018, 6:24 PM

May 18 2018

qcolombet added a comment to D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

Would it work to instead use RegUnits, so that we first collect all saved/restored RegUnits, and then check which registers have all their RegUnits saved?

May 18 2018, 2:56 PM
qcolombet accepted D43982: [GlobalISel][ARM] Adding HPR and QPR regclasses to FPRB regbank.
May 18 2018, 2:43 PM
qcolombet accepted D46640: [GlobalMerge] Exit early if only one global is to be merged.
May 18 2018, 2:38 PM
qcolombet added a comment to D46809: [GlobalISel] NFCI, Getting GlobalISel ~5% faster.

Nice finding Roman.

May 18 2018, 2:37 PM
qcolombet added a comment to D45204: [X86][MIPS][ARM] New machine instruction property 'isMoveReg'.

As you can see I left VMOVRRD ARM instruction as FIXME since it is more complicated copy.

May 18 2018, 1:58 PM

May 15 2018

qcolombet added a comment to D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

E.g., on x86 saving xmm0 is not enough to say that ymm0 is preservered even if ymm0 doesn’t have any other sub reg.

May 15 2018, 4:29 PM
qcolombet added a comment to D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

Just reread what you said, you indeed want the other way.
The problem is that the big register is not necessarily covered by the sub regs. So you have to check for that.

May 15 2018, 4:27 PM
qcolombet added a comment to D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

I was thinking about this the other way around: if all the subreg of a register are saved that doesn’t imply the register itself is saved.
The way you describe with adding the sub regs works great. I thought you wanted the other way :P

May 15 2018, 4:25 PM
qcolombet added inline comments to D46640: [GlobalMerge] Exit early if only one global is to be merged.
May 15 2018, 4:21 PM

May 11 2018

qcolombet updated subscribers of D46339: [GlobalISel][Legalizer] LegalizerInfo verifier: Follow Up.

Yes, please remove these.

May 11 2018, 3:05 PM
qcolombet added a comment to D46640: [GlobalMerge] Exit early if only one global is to be merged.

Looks sensible to me.

May 11 2018, 3:00 PM
qcolombet accepted D46339: [GlobalISel][Legalizer] LegalizerInfo verifier: Follow Up.
May 11 2018, 12:50 PM
qcolombet added a comment to D45204: [X86][MIPS][ARM] New machine instruction property 'isMoveReg'.

The general direction makes sense and I agree you probably don't want to implement the full isEquivalentTo stuff I was talking about.
A general pointer you might have missed. We already track copy like instructions through isRegSequenceLike, isExtractSubRegLike and so on. These are more complicated than just plain copies though (e.g., VMOVRRD should already be supported).

May 11 2018, 12:50 PM
qcolombet accepted D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

This loop (per what determineCalleeSaves does) will only add any of the registers that getCalleeSavedRegs() returns

May 11 2018, 12:31 PM

May 7 2018

qcolombet accepted D46490: [MachineVerifier][GlobalISel] Verifying generic extends and truncates.
May 7 2018, 12:59 PM
qcolombet accepted D46455: [MachineVerifier][GlobalISel] Checking that generic instrs have LLTs on all vregs.

Nice!

May 7 2018, 12:54 PM
qcolombet added inline comments to D46339: [GlobalISel][Legalizer] LegalizerInfo verifier: Follow Up.
May 7 2018, 12:50 PM
qcolombet accepted D45537: [CodeGenPrepare] Move Extension Instructions Through Logical And Shift Instructions.

Thanks for your patience, LGTM.

May 7 2018, 12:50 PM
qcolombet accepted D46039: Fix compile time hang in LSR.

LGTM with the nits fixed

May 7 2018, 12:43 PM
qcolombet requested changes to D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

Thanks for double checking. Now, I don't think some part of the patch makes sense. See my inline comment.
Basically, we should have to augment whatever determineCalleeSaved gives us, otherwise that means this information is just flawed.

May 7 2018, 12:40 PM

May 4 2018

qcolombet added inline comments to D46039: Fix compile time hang in LSR.
May 4 2018, 9:02 AM
qcolombet added a comment to D45156: [MachineVerifier] Verify the RegUsageInfo collected for the current function..

Good catch!

May 4 2018, 8:59 AM
qcolombet accepted D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization..

Hi Jonas,

May 4 2018, 8:55 AM
qcolombet added a comment to D46339: [GlobalISel][Legalizer] LegalizerInfo verifier: Follow Up.

With not llc you can still check specifics in FileCheck whereas with XFAIL you usually don't put checks at all.
In this case, I would expect a not llc + filecheck of some error message.

May 4 2018, 8:44 AM

May 3 2018

qcolombet added inline comments to D46339: [GlobalISel][Legalizer] LegalizerInfo verifier: Follow Up.
May 3 2018, 5:01 PM
qcolombet added a comment to D46338: [GlobalISel][Legalizer] LegalizerInfo verifier: checking that legalization rules cover all type indices.

Couple of nits on the tests.

May 3 2018, 4:57 PM