MatzeB (Matthias Braun)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 13 2013, 3:10 PM (197 w, 3 d)

Recent Activity

Today

MatzeB added a comment to D33562: MachineLICM: Add new condition for hoisting of caller preserved registers.

Instead of introducing yet another target callback, how about some simple heuristic like MRI.hasOneDef(Reg) && !MRI.getUsedPhysRegsMask().test(Reg)[1] allowing hoisting up to the point of that definition (which you can find out with MRI.def_instr_begin(Reg) or similar).

Fri, May 26, 2:19 PM
MatzeB accepted D32563: Add LiveRangeShrink pass to shrink live range within BB..

Register class logic looks good to me with the nitpicks below addressed.

Fri, May 26, 1:39 PM
MatzeB added a comment to D31188: [AntiDepBreaker] Use liveins as well in StartBlock.

Ok, now that r303938 / https://reviews.llvm.org/D32464 has landed could you please double check and then revert this?

You can hold off with this for now, somehow I broke some clang stage2 bots with that commit and I first have to investigate why.

Fri, May 26, 11:21 AM

Yesterday

MatzeB added a comment to D31188: [AntiDepBreaker] Use liveins as well in StartBlock.

Ok, now that r303938 / https://reviews.llvm.org/D32464 has landed could you please double check and then revert this?

Thu, May 25, 9:13 PM
MatzeB added a comment to D33579: Return a lit.Test.Result object from TestRunner's executeShTest().

Looks like an oversight in r189545. I am surprised this wasn't noticed before.

Thu, May 25, 5:11 PM
D31188: [AntiDepBreaker] Use liveins as well in StartBlock now requires changes to proceed.

Ok, now that r303938 / https://reviews.llvm.org/D32464 has landed could you please double check and then revert this?

Thu, May 25, 4:41 PM
MatzeB added a comment to D28404: IRGen: Add optnone attribute on function during O0.

FWIW, I think this makes sense.
Moving O0 and optnone get closer seems sensible. Even though -O3 with an optnone function indeed gives you different results today.
We are basically maintaining two things for the same "do not optimize" goal.
This obviously won't make O0 and optnone being the same in todays pass managers, but it is a step in the right direction.

Thu, May 25, 2:07 PM
MatzeB added a comment to D32304: Reduce printing values with default in MIR based codegen testing YAML.

If we make YAML:IO to write default values it will print every things including empty strings, for example consider test/CodeGen/X86/virtual-registers*.ll in that if we don't use -simplify-mir it will generate - { reg: '%edi', virtual-reg: '' } which makes POST-RA-NEXT check failed. This is why I have added -simplify-mir where required.
However in test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll I think adding -simplify-mir is not correct because it also removes basic block printing and for that particular case it is required that only one successor is present after certain statement so for that case I need to add fields which were not getting printed before but now they are getting printed.
Also before this patch we already do not print string with empty value so all these test-cases have been written in such manner that check for filed with empty string is not done so to preserve that I added -simplify-mir. Or we need to other way around.

Thu, May 25, 11:11 AM

Mon, May 22

MatzeB added inline comments to D33281: [Doc] Update how to install graphviz on macOS.
Mon, May 22, 3:51 PM
MatzeB added inline comments to D33281: [Doc] Update how to install graphviz on macOS.
Mon, May 22, 3:34 PM
MatzeB added inline comments to D33281: [Doc] Update how to install graphviz on macOS.
Mon, May 22, 3:31 PM

Fri, May 19

MatzeB added a comment to D32839: SimplifyLibCalls: Optimize wcslen.

Fixing getConstantStringInfo() here caused some changes in behavior in SelectionDAG memcpy lowering. I'll put in some more fixes before committing...

Fri, May 19, 2:29 PM
MatzeB added a comment to D32974: Verifier: Check wchar_size module flag..

ping

Fri, May 19, 11:55 AM
MatzeB updated the diff for D32839: SimplifyLibCalls: Optimize wcslen.
  • Add a test passing a pointer to an array with the wrong size to wcslen.
  • Add variants of tests in wcslen-1.ll in wcslen-3.ll that test with sizeof(wchar_t)==2.
  • (The clang patch to always emit wchar_size is approved.)
Fri, May 19, 11:40 AM

Thu, May 18

MatzeB added a comment to D24805: [GVNSink] Initial GVNSink prototype.

Looks all good from my side. But I am not the best person to review IR transformations, so would be good to wait for someone else.

Thu, May 18, 10:45 AM
MatzeB accepted D33303: AsmPrinter: mark the beginning and the end of a function in verbose mode.

Please wait a few more days for comments, and if nothing happens commit.

Thu, May 18, 10:05 AM

Wed, May 17

MatzeB added a comment to D33303: AsmPrinter: mark the beginning and the end of a function in verbose mode.

Nice, but I have some nitpicks:

Wed, May 17, 5:02 PM
MatzeB updated the diff for D32982: CodeGenModule: Always output wchar_size; check LLVM assumptions..

Add comment to test

Wed, May 17, 1:48 PM
MatzeB added a comment to D32982: CodeGenModule: Always output wchar_size; check LLVM assumptions..

ping. Do we really need wchar_t size in DataLayout to move this forward? I'm not convinced that is the right place for it...

Wed, May 17, 1:46 PM
MatzeB added a comment to D32563: Add LiveRangeShrink pass to shrink live range within BB..

patch to move the pass to x86 is proposed: https://reviews.llvm.org/D33294

Will have a separate patch to fix the REG_SEQUENCE handling once I get a test that I can include in unittesting. I suppose the fix is: ignore instruction whose def register size is no less than the sum of the use register sizes?

Wed, May 17, 1:00 PM

Tue, May 16

MatzeB accepted D33222: [LegacyPassManager] Remove TargetMachine constructors.

Thanks, this turned out to be a nice code simplification!
LGTM with nitpicks (including Chandlers comments) addressed.

Tue, May 16, 1:00 PM
MatzeB accepted D33181: Don't allow -optimize-regalloc=false with -regalloc given for anything other than 'fast'.

LGTM

Tue, May 16, 11:23 AM

Mon, May 15

MatzeB added reviewers for D33222: [LegacyPassManager] Remove TargetMachine constructors: qcolombet, echristo, chandlerc, bogner.

Thanks, this looks really interesting. Adding some more reviewers who may be interested in pass manager and codegen things.

Mon, May 15, 5:29 PM
MatzeB added a comment to D32304: Reduce printing values with default in MIR based codegen testing YAML.

In previous patch constant used to set default value for object property of type int64_t , uint64_t was not portable.
Due to

if (!SimplifyMIR)
      Out.setWriteDefaultValues(true);

some test cases need to be updated. Sorry previously I didn't run llvm-lit on whole test folder.

I don't understand this explanation. Can you give some example? It feels odd that you had to add -simplify-mir to all of those tests...

Mon, May 15, 11:12 AM
MatzeB added a comment to D32304: Reduce printing values with default in MIR based codegen testing YAML.

Also is there any way to test patch on build bot with out committing changes specially on different hardware?

Mon, May 15, 11:10 AM
MatzeB added a reviewer for D31821: Remove redundant copy in recurrences: wmi.

I'd like to hear @qcolombet's opinion about this, also +wmi who wrote the similar isRevCopyChain().

Mon, May 15, 10:59 AM
MatzeB added a comment to D33181: Don't allow -optimize-regalloc=false with -regalloc given for anything other than 'fast'.

This seems like a case for report_fatal_error() rarther than assert()

Mon, May 15, 9:40 AM

Fri, May 12

MatzeB accepted D32715: shrink-wrap: fix shrink-wrapping for no-return paths.

Okay I misread the code, looks like ArePointsInteresting() abort if Save or Restore is nullptr. So this seems fine.

Fri, May 12, 11:37 AM
MatzeB requested changes to D32715: shrink-wrap: fix shrink-wrapping for no-return paths.

Good find Francis. I am not convinced however the fix is save (see below). And for that matter the existing if (!Save) code path in FindIDom() seems to suffer the same problem.

Fri, May 12, 11:35 AM

Thu, May 11

MatzeB added a comment to D32304: Reduce printing values with default in MIR based codegen testing YAML.

I don't have commit access. Kindly commit it. Sorry for the trouble.

I think it's time that you ask Chris for it as you have a few patches in by now (see http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access )

Thu, May 11, 1:04 PM
MatzeB added a comment to D33037: [IfConversion] Keep the CFG updated incrementally in IfConvertTriangle.

Btw, without the fix we get the following result from IfConversion for the testcase:

body: |

bb.0:

bb.1:
  successors: %bb.2(0x40000000), %bb.1(0x00000000)

  BX_RET 14, 0

bb.2:

...

So bb.1 has both bb.2 and itself as successors, even if it only contains an unconditional return.

This is indeed wrong (but hard to catch with the MachineVerifier so it probably went unnoticed).

Thu, May 11, 10:58 AM
MatzeB accepted D32563: Add LiveRangeShrink pass to shrink live range within BB..

Accepting this now as it looks reasonably safe and fast to me (nitpicks below).

Thu, May 11, 10:52 AM
MatzeB accepted D32892: Handle a COPY with undef source operand in LowerCopy()..

LGTM

Thu, May 11, 10:26 AM
MatzeB added a comment to D33088: [LiveVariables] Switch Kill/Defs sets to be `DenseSet`.

And also slightly related: For the sort of queries that LiveVariables supports "Fast liveness checking for ssa-form programs" / https://doi.org/10.1145/1356058.1356064 would be a perfect fit. However given that the long term goal would be to replace all of this with LiveIntervalAnalysis, it's probably not worth pursuing that here.

Thu, May 11, 10:18 AM
MatzeB added a comment to D33088: [LiveVariables] Switch Kill/Defs sets to be `DenseSet`.
  • Easier fix: The critical edge splitting appears to be a part of PHIElimination that happens before anything else. If we would pull that out into a separate pass and perform it before computing LiveVariables, then we wouldn't need to go through the trouble of updating LiveVariables.

Reading the code a bit more it seems that the critical edge splitting logic is already depending on liveness information so it's not that easy to fix.

Thu, May 11, 10:13 AM
MatzeB accepted D33088: [LiveVariables] Switch Kill/Defs sets to be `DenseSet`.

And I should mark this green. LGTM

Thu, May 11, 10:11 AM
MatzeB added a comment to D33088: [LiveVariables] Switch Kill/Defs sets to be `DenseSet`.

If this turns out to be more common and we would want to address this on a more fundamental level, this is what we could do:

Thu, May 11, 10:11 AM
MatzeB added a comment to D33088: [LiveVariables] Switch Kill/Defs sets to be `DenseSet`.

Not much to say about the patch itself which looks fine.

Thu, May 11, 10:06 AM

Wed, May 10

MatzeB added a comment to D32742: [CodeGen] JumpMaps switch statement optimization (stub).

I like the general idea of jumpmaps as detailed on the mailinglist.

Wed, May 10, 1:50 PM
MatzeB added a comment to D32892: Handle a COPY with undef source operand in LowerCopy()..

I was thinking more about a minimal test that just focused on that one transformation that postrapseudos should do. Whether you also run the verifier or not shouldn't matter at that point. Should look something like this (I haven't actually tested this though, so you may need some tweaks):

# RUN: llc -mtriple=s390x-linux-gnu -mcpu=z13 -verify-machineinstrs -run-pass=postrapseudos -o - %s | FileCheck %s
---
# CHECK-LABEL: name: undef_copy
# CHECK: %r13d = KILL undef %r0d, implicit killed %r12q, implicit-def %r12q
name: undef_copy
tracksRegLiveness: true
body: |
  bb.0:
    liveins: %r12q
    %r13d = COPY undef %r0d, implicit killed %r12q, implicit-def %r12q
Wed, May 10, 9:36 AM

Tue, May 9

MatzeB added a reviewer for D33003: Add callee-saved registers as implicit uses in return instructions: qcolombet.

I really like the direction of this as it would get rid of the "pristine registers" and the "how to calculate live-outs of a return block" special cases.

Tue, May 9, 11:35 AM
MatzeB added inline comments to D32996: [IfConversion] Add missing check in IfConversion/canFallThroughTo.
Tue, May 9, 11:07 AM
MatzeB requested changes to D32563: Add LiveRangeShrink pass to shrink live range within BB..
Tue, May 9, 10:58 AM
MatzeB added a comment to D32802: (Was: Add checks so that -pre-RA-sched=list-ilp does not crash on SystemZ (Bug 32723). ->) Implement getRepRegClassFor() in SystemZ..

Just out of interested: What's the interested in these selection dag schedulers, I though they are deprecated and only SourceListDAGScheduler and possibly SchedulerDAGFast are of interest with the MachineScheduler around.

Is this because of the same reasons (or same benchmark?) that motivated https://reviews.llvm.org/D32563 ?

This was just a bug I discovered with llvm-stress. As long as this option is around, it shouldn't mean that the compiler crashes when it is used, right? (Perhaps an exit with some kind of message explaining that this scheduler is unsupported might due instead...?)

Tue, May 9, 10:04 AM
MatzeB added a comment to D32802: (Was: Add checks so that -pre-RA-sched=list-ilp does not crash on SystemZ (Bug 32723). ->) Implement getRepRegClassFor() in SystemZ..

Just out of interested: What's the interested in these selection dag schedulers, I though they are deprecated and only SourceListDAGScheduler and possibly SchedulerDAGFast are of interest with the MachineScheduler around.

Is this because of the same reasons (or same benchmark?) that motivated https://reviews.llvm.org/D32563 ?

This was just a bug I discovered with llvm-stress. As long as this option is around, it shouldn't mean that the compiler crashes when it is used, right? (Perhaps an exit with some kind of message explaining that this scheduler is unsupported might due instead...?)

Tue, May 9, 9:59 AM
MatzeB added a comment to D32802: (Was: Add checks so that -pre-RA-sched=list-ilp does not crash on SystemZ (Bug 32723). ->) Implement getRepRegClassFor() in SystemZ..

Just out of interested: What's the interested in these selection dag schedulers, I though they are deprecated and only SourceListDAGScheduler and possibly SchedulerDAGFast are of interest with the MachineScheduler around.

Tue, May 9, 9:53 AM
MatzeB added a comment to D32892: Handle a COPY with undef source operand in LowerCopy()..

ping!

This is barely one workday old (in my timezone).

Tue, May 9, 9:31 AM

Mon, May 8

MatzeB added a comment to D32982: CodeGenModule: Always output wchar_size; check LLVM assumptions..
In D32982#749215, @rnk wrote:

This seems like the kind of thing we would normally express in DataLayout.

Mon, May 8, 5:14 PM
MatzeB added a dependent revision for D32839: SimplifyLibCalls: Optimize wcslen: D32982: CodeGenModule: Always output wchar_size; check LLVM assumptions..
Mon, May 8, 3:39 PM
MatzeB added a dependency for D32982: CodeGenModule: Always output wchar_size; check LLVM assumptions.: D32839: SimplifyLibCalls: Optimize wcslen.
Mon, May 8, 3:39 PM
MatzeB created D32982: CodeGenModule: Always output wchar_size; check LLVM assumptions..
Mon, May 8, 3:39 PM
MatzeB added inline comments to D32839: SimplifyLibCalls: Optimize wcslen.
Mon, May 8, 2:08 PM
MatzeB updated the diff for D32839: SimplifyLibCalls: Optimize wcslen.
  • Check type in !Array zeroinitializer case. (Technically this shouldn't be necessary as it would be UB otherwise, but optimizing based on this will hardly buy us anything).
Mon, May 8, 2:08 PM
MatzeB created D32974: Verifier: Check wchar_size module flag..
Mon, May 8, 11:15 AM

Fri, May 5

MatzeB added a comment to D31262: MIParser/MIRPrinter: Compute block successors if not explicitely specified.

Thanks for the review.

Fri, May 5, 2:16 PM
MatzeB updated subscribers of D32839: SimplifyLibCalls: Optimize wcslen.
Fri, May 5, 1:09 PM
MatzeB added inline comments to D32839: SimplifyLibCalls: Optimize wcslen.
Fri, May 5, 1:08 PM
MatzeB updated the diff for D32839: SimplifyLibCalls: Optimize wcslen.
  • Add functionality to TargetLibraryInfo to determine the size of wchar_t for a target triple (an upcoming clang patch will introduce an assert() on the clang side to catch when they become out of sync).
  • getConstantDataArrayInfo() now returns Offset+Length into the ConstantDataArray and corrects handling of zeroinitializers
Fri, May 5, 1:04 PM

Thu, May 4

MatzeB accepted D32661: Remove stale live-ins in the branch folder.

I would consider it okay if the live-in lists contain more registers. I consider it a code-quality/optimization problem but in principle I don't think passes should fail because of it, so there would be a bug in IfConversion when it creates invalid code because of extra live-in registers.

Thu, May 4, 3:32 PM
MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

We add all callee saved registers as live-in on all paths from the restores to the exit, so this problem shouldn't affect us. Actually the anti-dep breaker was the reason why we did it in the first place.

The second loop in function AggressiveAntiDepBreaker::StartBlock tries to do the work you described. Unfortunately it doesn't work with shrink wrapping. It says:

This should be done in PEI, not in the anti-dependency breaker. This is where the CSR registers get the concrete saving/restoring code. Passes running afterwards should not need to know about what register is CSR or not (except pristine registers, but that's general enough).

Note that this is true for live-ins, however we don't explicitely safe live-out lists in the basic blocks. So for return blocks you have to take all CSR registers (depending on what you do with/without pristines). (See also https://reviews.llvm.org/D32464 where I partially rediscovered that fact).

Can your patch in D32464 also fix this problem?

Possibly, try it out.

Thu, May 4, 2:48 PM
MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

We add all callee saved registers as live-in on all paths from the restores to the exit, so this problem shouldn't affect us. Actually the anti-dep breaker was the reason why we did it in the first place.

The second loop in function AggressiveAntiDepBreaker::StartBlock tries to do the work you described. Unfortunately it doesn't work with shrink wrapping. It says:

This should be done in PEI, not in the anti-dependency breaker. This is where the CSR registers get the concrete saving/restoring code. Passes running afterwards should not need to know about what register is CSR or not (except pristine registers, but that's general enough).

Note that this is true for live-ins, however we don't explicitely safe live-out lists in the basic blocks. So for return blocks you have to take all CSR registers (depending on what you do with/without pristines). (See also https://reviews.llvm.org/D32464 where I partially rediscovered that fact).

Can your patch in D32464 also fix this problem?

Thu, May 4, 2:46 PM
MatzeB added inline comments to D32839: SimplifyLibCalls: Optimize wcslen.
Thu, May 4, 1:48 PM
MatzeB updated the diff for D32839: SimplifyLibCalls: Optimize wcslen.
  • Determine wchar_t size based on the wcslen declaration
  • Improve/fix tests
Thu, May 4, 1:45 PM
MatzeB updated the diff for D32837: TargetLibraryInfo: Introduce wcslen.
  • Add missing wcslen declaration to unittests/Analysis/TargetLibraryInfoTest.cpp
Thu, May 4, 1:17 PM
MatzeB added inline comments to D32839: SimplifyLibCalls: Optimize wcslen.
Thu, May 4, 11:45 AM
MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

Yeah I like that way of modeling things explicitely. Unfortunately that is not how most llvm targets work today.

Would it be reasonable to implement that for all targets?

I think it would work and I think it would make the representation a bit cleaner. If there happened to be a target with hundreds of CSRs that would be a lot of extra operands, on the other hand that target already has hundreds of entries in the CSR list anyway.

Thu, May 4, 10:56 AM

Wed, May 3

MatzeB updated the diff for D31262: MIParser/MIRPrinter: Compute block successors if not explicitely specified.
  • Move MIRPrinter.h to include/llvm/CodeGen to avoid ../ relative path.
Wed, May 3, 6:50 PM
MatzeB updated the diff for D31262: MIParser/MIRPrinter: Compute block successors if not explicitely specified.

I'm still not completely convinced. But in the interest of compromise and getting this patch through:

Wed, May 3, 6:47 PM
MatzeB updated the diff for D32839: SimplifyLibCalls: Optimize wcslen.

Add some tests that I missed to include in the last revision.

Wed, May 3, 6:01 PM
MatzeB added a dependent revision for D32837: TargetLibraryInfo: Introduce wcslen: D32839: SimplifyLibCalls: Optimize wcslen.
Wed, May 3, 4:45 PM
MatzeB added a dependency for D32839: SimplifyLibCalls: Optimize wcslen: D32837: TargetLibraryInfo: Introduce wcslen.
Wed, May 3, 4:45 PM
MatzeB created D32839: SimplifyLibCalls: Optimize wcslen.
Wed, May 3, 4:45 PM
MatzeB created D32837: TargetLibraryInfo: Introduce wcslen.
Wed, May 3, 4:38 PM
MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

So for return blocks you have to take all CSR registers (depending on what you do with/without pristines). (See also https://reviews.llvm.org/D32464 where I partially rediscovered that fact).

We (Hexagon) add all the non-pristine CSRs as implicit uses of the return instructions.

Wed, May 3, 1:40 PM
MatzeB added a comment to D32464: LivePhysRegs: Fix addLiveOutsNoPristines() for return blocks past PEI.

ping

Wed, May 3, 1:21 PM
MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

We add all callee saved registers as live-in on all paths from the restores to the exit, so this problem shouldn't affect us. Actually the anti-dep breaker was the reason why we did it in the first place.

The second loop in function AggressiveAntiDepBreaker::StartBlock tries to do the work you described. Unfortunately it doesn't work with shrink wrapping. It says:

This should be done in PEI, not in the anti-dependency breaker. This is where the CSR registers get the concrete saving/restoring code. Passes running afterwards should not need to know about what register is CSR or not (except pristine registers, but that's general enough).

Wed, May 3, 1:20 PM

Tue, May 2

MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

1 The function MFI.getPristineRegs(MF) gets all registers actually saved in the function, not necessary at the prolog position if shrink wrapping has occurred.

Tue, May 2, 4:25 PM
MatzeB accepted D31195: PEI: Skip dead objects when looking at CSRs.

Seems okay to go ahead with this.
However looking around the code it seems we need similar ifs in various other loops inside the file.

Tue, May 2, 11:55 AM

Mon, May 1

MatzeB added a comment to D31262: MIParser/MIRPrinter: Compute block successors if not explicitely specified.

In a nutshell, I am saying that we shouldn't conflict two goals: editing and printing. Printing should be full fledged, editing should be minimal.

Mon, May 1, 5:03 PM
MatzeB added a comment to D32563: Add LiveRangeShrink pass to shrink live range within BB..
In D32563#742848, @wmi wrote:

+ Andy, for the history on pre-RA-sched and misched.

Mon, May 1, 4:52 PM
MatzeB added a reviewer for D32622: ARM: Compute MaxCallFrame size early: qcolombet.
Mon, May 1, 4:43 PM
MatzeB updated the diff for D32622: ARM: Compute MaxCallFrame size early.
  • Change MachineVerifier to work if reserved registers are not frozen yet.
Mon, May 1, 4:43 PM
MatzeB added a comment to D32622: ARM: Compute MaxCallFrame size early.

I had to change GlobalISel to call TLI->finalizeLowering() late in InstructionSelect instead of after IRTranslator. While most ADJCALLSTACK/CALL instructions are already visible after IRTranslator we may miss operations getting implemented by library calls later. It also seems to delay freezing the reserved registers + extra adjustments until after ISel is finished. This also matches the current SelectionDAG behavior.

+Reviewers Ahmed because of the finalizeLowering/freezeReg movement.

Mon, May 1, 11:37 AM
MatzeB added inline comments to D32058: Add a section about simplifying .mir tests.
Mon, May 1, 11:25 AM
MatzeB updated the diff for D32622: ARM: Compute MaxCallFrame size early.

I had GlobalISel disabled in my earlier tests (cmake cache from times when it wasn't enabled yet).

Mon, May 1, 11:23 AM
MatzeB added a comment to D32570: MachineFrameInfo: Track whether MaxCallFrameSize is computed yet.

Hi Matthias,

however this fails various targets that need to be fixed first.

What are the PRs for that?

Is this something you plan to fix? (just asking :))

I fixed ARM (see dependent patches). I do not plan to fix the other targets but will file PRs.

Mon, May 1, 10:22 AM
MatzeB added inline comments to D32058: Add a section about simplifying .mir tests.
Mon, May 1, 10:18 AM

Fri, Apr 28

MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

In any case as far as I understand it this patch is about not renaming one of the restored registers (so we do not suddenly restore the callee save into some unrelated register that happens to be free). So it's not about liveness but about which registers are legal to rename/recolor.

Then what's the problematic situation? It was exactly my understanding that we renamed a callee-saved register defined by the restore instruction. What else could be the problem?

Fri, Apr 28, 3:44 PM
MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

This hits on a known problem in llvms CodeGen today: There is no good way to differentiate between a register use/def that is required because of ABI (or other) constraints and just a normal assignment by the register allocator. Only the latter can be renamed/recolored which makes it tricky to it correctly today.

I personally don't care about AggressiveAntiDepBreaker too much. However this "fix" seems pretty pessimistic/conservative to me: As I understand it you just exclude all the callee saves from renaming. I think targets that rely on the anti dep breaker (seems to be used by PowerPC/Hexagon today) should at least do some benchmarking before accepting this.

The patch also checks (RegRefs.count(AntiDepReg) == 1), with this condition, it almost ensures the instruction is callee saved register restore instruction, if it is not, it should be some other rare cases.

Fri, Apr 28, 3:29 PM
MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

All callee saves? That would be new to me. I think we only do that for callee saves that are saved/restored *somewhere* in the function, the rest is considered "pristine" and not explicitely mentioned in the live-in lists.

Yes, that's what I meant---the ones that were actually clobbered in the function.

Fri, Apr 28, 3:27 PM
MatzeB added a comment to D31908: [AntiDepBreaker] Don't rename callee saved register restore instruction.

We add all callee saved registers as live-in on all paths from the restores to the exit, so this problem shouldn't affect us. Actually the anti-dep breaker was the reason why we did it in the first place.

Fri, Apr 28, 3:19 PM
MatzeB added inline comments to D32304: Reduce printing values with default in MIR based codegen testing YAML.
Fri, Apr 28, 12:49 PM
MatzeB updated the diff for D32570: MachineFrameInfo: Track whether MaxCallFrameSize is computed yet.

bring back TODO comment that was deleted by accident.

Fri, Apr 28, 12:41 PM
MatzeB updated the diff for D32570: MachineFrameInfo: Track whether MaxCallFrameSize is computed yet.

bring back a TODO comment that was accidentally deleted.

Fri, Apr 28, 12:39 PM
MatzeB accepted D32650: Properly handle PHIs with subregisters in UnreachableBlockElim.

LGTM.

Fri, Apr 28, 12:29 PM
MatzeB added a comment to D32621: TargetLowering: Add finalizeLowering() function; NFC.
In D32621#741035, @rnk wrote:

Oops, I went straight to the diff and missed Matt's comment.

Fri, Apr 28, 12:26 PM
MatzeB added inline comments to D32650: Properly handle PHIs with subregisters in UnreachableBlockElim.
Fri, Apr 28, 11:07 AM
MatzeB added inline comments to D32622: ARM: Compute MaxCallFrame size early.
Fri, Apr 28, 9:47 AM
MatzeB added inline comments to D32622: ARM: Compute MaxCallFrame size early.
Fri, Apr 28, 9:45 AM

Thu, Apr 27

MatzeB added inline comments to D32621: TargetLowering: Add finalizeLowering() function; NFC.
Thu, Apr 27, 5:54 PM