Page MenuHomePhabricator

Fix PR26655: Bail out if all regs of an inst BUNDLE have the correct kill flag
ClosedPublic

Authored by mgrang on Feb 17 2016, 3:39 PM.

Details

Summary

While setting kill flags on instructions inside a BUNDLE, we bail out as soon
as we set kill flag on a register. But we are missing a check when all the
registers already have the correct kill flag set. We need to bail out in that
case as well.

This patch refactors the old code and simply makes use of the addRegisterKilled
function in MachineInstr.cpp in order to determine whether to set/remove kill
on an instruction.

Diff Detail

Repository
rL LLVM

Event Timeline

mgrang updated this revision to Diff 48246.Feb 17 2016, 3:39 PM
mgrang retitled this revision from to Fix PR26655: Incorrect liveness for bundled instructions with CPSR<kill> marker.
mgrang updated this object.
mgrang set the repository for this revision to rL LLVM.
mgrang added a subscriber: llvm-commits.
mgrang updated this object.Feb 17 2016, 3:57 PM
mgrang added reviewers: stoklund, grosbach.

A gentle reminder for reviews please.

mgrang added a reviewer: asl.Feb 18 2016, 12:42 PM

Reviews please.

davide added a subscriber: davide.Feb 19 2016, 12:52 PM

Sorry if I point this out, but it already happened several times in the past (on patches where I was listed as reviewer). It's a bit unpleasant pinging each day for a patch. I'm not sure if the developer's policy document what's the timeout for a ping, but I expect 1 week to be a reasonable timeframe.
Also, keep adding (somewhat unrelated) reviewers, I don't think that helps.
As an added bonus, 3.8 is coming out anytime soon, and most of us are very busy trying to fix/review critical regressions, so a little bit of delay is somehow expected =)

Thanks for the suggestions Davide and apologies for being too impatient :)
As for the reviewers, I see that the ones I have added have contributed to the source file I am making changes to. So not sure what you meant by "unrelated".

Ping3 for reviews please.

mgrang added a comment.Mar 3 2016, 9:27 AM

Ping 4 for reviews please.

t.p.northover edited edge metadata.Mar 8 2016, 9:34 AM

Hi,

Sorry it's taken so long to review this, but it doesn't look right to me. The IT placement pass is producing what looks like correct code:

BUNDLE %ITSTATE<imp-def,dead>, %R0<imp-def>, %D16<imp-def>, %R2<imp-use>, %CPSR<imp-use,kill>, %D17<imp-use,kill>
  * t2IT 12, 4, %ITSTATE<imp-def>
  * %R0<def> = t2MOVr %R2, pred:12, pred:%CPSR, opt:%noreg, %ITSTATE<imp-use,internal>
  * %D16<def> = VMOVD %D17<kill>, pred:12, pred:%CPSR<kill>, %ITSTATE<imp-use,kill,internal>

And then the Machine Scheduler adds what looks like a spurious <kill> flag to the CPSR usage:

BUNDLE %ITSTATE<imp-def,dead>, %R0<imp-def>, %D16<imp-def>, %R2<imp-use>, %CPSR<imp-use,kill>, %D17<imp-use,kill>
  * t2IT 12, 4, %ITSTATE<imp-def>
  * %R0<def> = t2MOVr %R2, pred:12, pred:%CPSR<kill>, opt:%noreg, %ITSTATE<imp-use,internal>
  * %D16<def> = VMOVD %D17<kill>, pred:12, pred:%CPSR<kill>, %ITSTATE<imp-use,kill,internal>

I think there's some kind of bug in the scheduler, or how ARM interacts with it. Or possibly ARM's entire usage of bundles (if the rest of LLVM strongly thinks the contents occur simultaneously or something), but that would be a shame.

Cheers.

Tim.

Thanks a lot for the review and comments Tim. I will take another look at this.

mgrang updated this revision to Diff 50764.Mar 15 2016, 1:26 PM
mgrang retitled this revision from Fix PR26655: Incorrect liveness for bundled instructions with CPSR<kill> marker to Fix PR26655: Bail out if all the registers of an instruction in a BUNDLE already have the correct kill flag..
mgrang updated this object.
mgrang edited reviewers, added: pete; removed: asl, grosbach, stoklund, resistor, weimingz.
mgrang updated this object.
mgrang removed rL LLVM as the repository for this revision.
t.p.northover added inline comments.Mar 16 2016, 9:49 AM
lib/CodeGen/ScheduleDAGInstrs.cpp
1206 ↗(On Diff #50764)

I think this goes wrong if the last instruction in the bundle doesn't kill Reg: the MachineOperand loop never clears KillStateAlready set and so we bail.

Perhaps a more suggestive name for this variable would be SeenKill.

MatzeB added inline comments.Mar 16 2016, 11:42 AM
lib/CodeGen/ScheduleDAGInstrs.cpp
1232–1235 ↗(On Diff #50764)

Indentation is wrong.
I wonder if "NewKillState = false" wouldn't be more consistent than "return", otherwise we will update all operands in the NewKillState==false case, but stop after we encountered an instruction using the register in the NewKillState==true case.

mgrang updated this revision to Diff 50857.Mar 16 2016, 1:23 PM
mgrang updated this object.

Sorry for messing you around here, but we've just noticed there are functions in MachineInstr that should be able to handle this for you. Specifically addRegisterKilled (make sure AddIfNotFound is false) and clearRegisterKills can replace the inner loop entirely (we think). They should also handle register aliases, which is probably a good thing.

Cheers.

Tim (after a chat with Matthias).

mgrang updated this revision to Diff 51103.Mar 18 2016, 9:15 PM
mgrang updated this object.
mgrang set the repository for this revision to rL LLVM.

Thanks Tim and Matthias for the suggestions.

I have updated my patch to make use of the addRegisterKilled and clearRegisterKills functions.
However, I notice that the addRegisterKilled function does not check if the operand is a debug value or if it is setting an internal flag not visible outside the bundle. I have now added these checks to addRegisterKilled function.

Tim/Matthias,
Could you please review this patch?

Thanks,
Mandeep

Yep, sorry about the delay. That addition to addRegisterKilled needs some careful thought, since this isn't the only user.

The debug operand is probably fine, but it seems completely unobvious to me that all other users will want to skip internal reads, so we've got to think about all other call-sites. Not helped by the fact that I can't even see why it's necessary in this case yet.

I haven't forgotten though!

Tim.

mgrang updated this revision to Diff 54231.Apr 19 2016, 11:23 AM
mgrang retitled this revision from Fix PR26655: Bail out if all the registers of an instruction in a BUNDLE already have the correct kill flag. to Fix PR26655: Bail out if all regs of an inst BUNDLE have the correct kill flag.
mgrang updated this object.

Sorry Tim for the delay in addressing your comments - was busy with an upcoming release.

So I ran the entire set of correctness tests in our validation framework without the check for isInternalRead() and I did not see any extra regressions. Also there are no extra unit test failures without this check. So I guess the check for isInternalRead() is really not needed and can be safely removed.

On the other hand the check for isDebug() is needed otherwise the following unit test fails:

******************** TEST 'LLVM :: CodeGen/MIR/ARM/sched-it-debug-nodes.mir' FAILED ********************
Script:
--
build/llvm/./bin/llc -mtriple thumbv7 -start-after if-converter -print-before=post-RA-sched -print-after=post-RA-sched src/llvm/test/CodeGen/MIR/ARM/sched-it-debug-nodes.mir -o /dev/null 2>&1 | build/llvm/./bin/FileCheck src/llvm/test/CodeGen/MIR/ARM/sched-it-debug-nodes.mir
--
Exit Code: 134

Command Output (stderr):
--
build/llvm/test/CodeGen/MIR/ARM/Output/sched-it-debug-nodes.mir.script: line 1: 41373 Aborted                 build/llvm/./bin/llc -mtriple thumbv7 -start-after if-converter -print-before=post-RA-sched -print-after=post-RA-sched src/llvm/test/CodeGen/MIR/ARM/sched-it-debug-nodes.mir -o /dev/null 2>&1
     41374 Done                    | build/llvm/./bin/FileCheck src/llvm/test/CodeGen/MIR/ARM/sched-it-debug-nodes.mir

Gentle reminder for reviews please.

Tim/Matthias,
Could you please review this patch?

Thanks,
Mandeep

MatzeB accepted this revision.May 5 2016, 8:26 PM
MatzeB added a reviewer: MatzeB.

So I ran the entire set of correctness tests in our validation framework without the check for isInternalRead() and I did not see any extra regressions. Also there are no extra unit test failures without this check. So I guess the check for isInternalRead() is really not needed and can be safely removed.

You have to be carefull here, I think there are just no in-tree targets at the moment that use bundles before register allocation. Having said that I looked at the code and I believe it was checking the first instruction of a bundle only, the first instruction is the BUNDLE summary instruction and should not have any internal reads anyway so with this reasoning it should be fine to ignore it.

I believe the check for DebugValue nodes can be done outside (see below). With the issues below addressed this looks good to me.

lib/CodeGen/MachineInstr.cpp
1944–1950 ↗(On Diff #54231)

Why not check MI.isDebugValue() at the caller?

lib/CodeGen/ScheduleDAGInstrs.cpp
1200 ↗(On Diff #54231)

Use a reference to pass TRI as it must not be nullptr.

test/CodeGen/Thumb2/thumb2-cpsr-liveness.ll
1 ↗(On Diff #54231)

It would be nice to add a comment on what the test is testing.

This revision is now accepted and ready to land.May 5 2016, 8:26 PM
This revision was automatically updated to reflect the committed changes.