Prevent hoisting the instruction whose def might be clobbered by the terminator

Sasa Stankovic Sasa.Stankovic at imgtec.com
Wed Jun 4 15:33:14 PDT 2014


Hi Quentin,

>The test case needs a few tweaks. Indeed, relying on basic block names in assembly is a bit fragile. You can rely on jump/branch instead (or anything else that make sense for you :)).

I updated the test. Test uses a workaround - in order to match the correct "b" (branch) instruction, I needed to add the CHECK for "addiu" instruction that precedes it. Otherwise, if I remove the check for "addiu", test will match first "b" instruction encountered.

Regards,
Sasa
________________________________________
From: Quentin Colombet [qcolombet at apple.com]
Sent: Tuesday, June 03, 2014 10:43 PM
To: Sasa Stankovic
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: Prevent hoisting the instruction whose def might be clobbered by the terminator

Hi Sasa,

Thanks for the updated version.
The test case needs a few tweaks. Indeed, relying on basic block names in assembly is a bit fragile. You can rely on jump/branch instead (or anything else that make sense for you :)).

Thanks,

Q.


Envoyé de mon iPhone

> Le 2 juin 2014 à 15:09, Sasa Stankovic <Sasa.Stankovic at imgtec.com> a écrit :
>
> Hi Quentin,
>
> Attached is the patch with the code-gen test case.  The test checks that at the start of two basic blocks there are identical instructions that define register $at (test actually checks for $1, but $at and $1 are the same register).
>
> Regards,
> Sasa
> ________________________________________
> From: Quentin Colombet [qcolombet at apple.com]
> Sent: Saturday, May 31, 2014 12:18 AM
> To: Sasa Stankovic
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: Prevent hoisting the instruction whose def might be clobbered by the terminator
>
> Hi Sasa,
>
> On May 30, 2014, at 2:14 PM, Sasa Stankovic <Sasa.Stankovic at imgtec.com<mailto:Sasa.Stankovic at imgtec.com>> wrote:
>
> Hi Quentin,
>
> I removed the check "if (MO.isImplicit())". I also reduced the test case (using bugpoint).
> Thanks!
>
>
> Also, that would be nice to check the actual code generation. That way, the REQUIRES line wouldn’t be needed and the problem would be more obvious.
>
> What does "REQUIRES: asserts" mean?  I looked for it in the LLVM documentation but didn't find anything.
> In your test case you are using -print-machineinstrs=branch-folder, which uses the debug output of the the related pass. That output is available only in Asserts build. Therefore, if your test case relies on that output, it means it has to “say” that it requires an assert build to work. This line is here for that.
>
> The bottom line is, your test case still misses that line :).
>
> That said, again I’d rather see a code gen test case, if you are able to write the related CHECK lines for the assembly.
>
> Thanks,
> -Quentin
>
>
> Regards,
> Sasa
>
> ________________________________________
> From: Quentin Colombet [qcolombet at apple.com<mailto:qcolombet at apple.com>]
> Sent: Thursday, May 29, 2014 10:10 PM
> To: Sasa Stankovic
> Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
> Subject: Re: Prevent hoisting the instruction whose def might be clobbered by the terminator
>
> Hi Sasa,
>
> A couple of remarks:
> #1
> +      if (MO.isImplicit()) {
> +        // If the terminator implicitly defines a register, make sure we don't
>
> Remove the condition.
> At this point the def is dead, however, implicit or not, it will still clobber any live variable in that register.
>
> #2
> +; RUN: llc < %s -march=mipsel -relocation-model=pic -O3 -print-machineinstrs=branch-folder -o /dev/null 2>&1 | FileCheck %s
> The current test needs, I guess, a line with ; REQUIRES: asserts
>
> #3
> Could you reduce the test case please?
> You can use bugpoint to help you.
>
> Also, that would be nice to check the actual code generation. That way, the REQUIRES line wouldn’t be needed and the problem would be more obvious.
>
> #4
> +; CHECK: Machine code for function readLumaCoeff8x8_CABAC:
> +; CHECK-NOT: Live Ins: {{.*}}%AT{{.*}}
>
> Insert the check lines earlier in the test case.
>
> Thanks,
> -Quentin
>
> On May 29, 2014, at 12:12 PM, Sasa Stankovic <Sasa.Stankovic at imgtec.com<mailto:Sasa.Stankovic at imgtec.com><mailto:Sasa.Stankovic at imgtec.com>> wrote:
>
> Hi Quentin,
>
> I added the test. The function is taken from the ldecod MultiSource test. The test uses
> -print machineinstrs=branch-folder option and checks that $at is not live-in at the start of any block after the pass.
>
> I also notified Evan Cheng (the author of the code that my patch changes) about the patch, and here is what he replied:
>
> <<<<
> Ok, I think the original code is just wrong. It's trying to be conservative by disallowing hoisting in case the terminator defines a register. However, it should not be checking if the def is dead.
>
> So your fix should be either 1) just disallow hoisting or 2) add the def to the defs list. It shouldn't matter if it's implicit.
>
> So either
>
> -    } else if (!MO.isDead())
> -      // Don't try to hoist code in the rare case the terminator defines a
> -      // register that is later used.
> -      return MBB->end();
> +    } else {
> +      // Don't try to hoist code in the rare case the terminator defines a
> +      // register that is later used.
> +      return MBB->end();
> +    }
>
> Or
>
> -    } else if (!MO.isDead())
> -      // Don't try to hoist code in the rare case the terminator defines a
> -      // register that is later used.
> -      return MBB->end();
> +    } else {
> +      // If the terminator implicitly defines a register, make sure we don't
> +      // hoist the instruction whose def might be clobbered by the terminator.
> +      for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI)
> +        Defs.insert(*AI);
> +    }
>
>
> Evan
> <<<<
>
> Regards,
> Sasa
> ________________________________________
> From: Quentin Colombet [qcolombet at apple.com<mailto:qcolombet at apple.com><mailto:qcolombet at apple.com>]
> Sent: Thursday, May 29, 2014 12:02 AM
> To: Sasa Stankovic
> Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu><mailto:llvm-commits at cs.uiuc.edu>
> Subject: Re: Prevent hoisting the instruction whose def might be clobbered by the terminator
>
> Thanks for the update.
>
> Could you add a test case, please?
>
> I’d like to see the patch in action :).
>
> Thanks,
> -Quentin
>
> On May 28, 2014, at 2:58 PM, Sasa Stankovic <Sasa.Stankovic at imgtec.com<mailto:Sasa.Stankovic at imgtec.com><mailto:Sasa.Stankovic at imgtec.com><mailto:Sasa.Stankovic at imgtec.com>> wrote:
>
> Hi Quentin,
>
> The patch is attached. Sorry for omitting it.
>
> When does the definition of $at appear on the branches?
>
> The tablegen definitions for direct branches specify that $at is defined by branches. So code hoisting pass should see that $at is implicitly defined by a branch.
>
> Regards,
> Sasa
> ________________________________________
> From: Quentin Colombet [qcolombet at apple.com<mailto:qcolombet at apple.com><mailto:qcolombet at apple.com><mailto:qcolombet at apple.com>]
> Sent: Wednesday, May 28, 2014 11:41 PM
> To: Sasa Stankovic
> Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu><mailto:llvm-commits at cs.uiuc.edu><mailto:llvm-commits at cs.uiuc.edu>
> Subject: Re: Prevent hoisting the instruction whose def might be clobbered by   the terminator
>
> Hi Sasa,
>
> I do not see a patch attached, so it is hard to comment on the fix itself :).
> That said, based on your description, I have one main question.
>
> When does the definition of $at appear on the branches?
>
> If it is before the hoisting, yes this is a bug in the hoisting. If it is after, then this is a bug in the expansion. Preventing the hoisting is just a way to hide this bug.
>
> The proper fix in that case would be IMHO to save $at in a temporary register and use it to restore the value in $at. (That suggestion assumes that $at can temporarily hold a “bad” value, this might not be acceptable for your target for instance in case of interrupt.)
> Now, if $at is not a fixed register (i.e., fixed by ABI or encoding), maybe the expansion occurs too late (i.e., after register allocation).
>
> Cheers,
> -Quentin
>
> On May 28, 2014, at 2:14 PM, Sasa Stankovic <Sasa.Stankovic at imgtec.com<mailto:Sasa.Stankovic at imgtec.com><mailto:Sasa.Stankovic at imgtec.com><mailto:Sasa.Stankovic at imgtec.com><mailto:Sasa.Stankovic at imgtec.com>> wrote:
>
> Hi,
>
> Attached is the patch that checks for registers implicitly defined by the terminator, to prevent hoisting the instruction whose def might be clobbered by the terminator.
>
> The problem that the attached patch fixes was encountered while stress testing MIPS long branches on LLVM test suite (description of MIPS long branches, needed to better understand why the test failed is given below). We passed the MIPS-specific command line option to llc that expands all direct branches to long branches.
>
> The situation that caused test to fail is as follows. We have 3 blocks (say B1, B2 and B3), where B1 is the only predecessor of B2 and B3, and there is identical code at the start of B2 and B3. This identical code is removed from B2 and B3, and moved to B1.
>
> The problem is that instruction that defines $at was moved to B1, but the instruction that uses $at was left in B2. So when the branch at the end of B1 is expanded to long branch, it clobbers $at and B2 will use the wrong value of $at.
>
> Here is a short description of MIPS long branches, and how register $at is used. MIPS has direct branch instructions with the 18-bit offset. When the actual branch offset cannot fit into 18-bit field, MipsLongBranch pass (run as the last pass before code emission) replaces every such direct branch with the following long branch sequence:
>
> 1:   $longbr:
> 2:       addiu $sp, $sp, -8
> 3:       sw $ra, 0($sp)
> 4:       lui $at, %hi($tgt - $baltgt)
> 5:       bal $baltgt
> 6:       addiu $at, $at, %lo($tgt - $baltgt)
> 7:   $baltgt:
> 8:       addu $at, $ra, $at
> 9:       lw $ra, 0($sp)
> 10:      addiu $sp, $sp, 8
> 11:      jr $at
> 12:  $fallthrough:
>
> If you are not familiar with the MIPS assembly, here is what this code does. First the stack pointer $sp is incremented by 8 bytes and the original return address $ra is saved on stack. The instructions in lines 4 and 6 load $at register with the value $tgt - $baltgt, which is the offset of the branch target $tgt from the label $baltgt. BAL instruction is a function call and it jumps to the label $baltgt; after the call is executed the return address register $ra has the value $baltgt (note that MIPS has branch delay slots, so that return address is the second instruction following the branch). ADDU instruction adds "$baltgt" and "$tgt - $baltgt", so that $at will contain the address of branch target $tgt. At the end $ra is restored from stack, stack pointer is also restored, and the code jumps to the target basic block through the indirect jump "jr $at".
>
> This code uses registers $ra and $at. $ra is saved on stack and restored, but we cannot do the same for $at since the code jumps to target through $at. So register $at is always clobbered by the long branch. To prevent that this affects the surrounding code, MIPS direct branches implicitly define register $at (that is, tablegen definitions for MIPS direct branches specify that they define register $at). This ensures that virtual register defined in one block and used in successor block won't be allocated in register $at, thus making it safe to expand ordinary branch to long branch.
>
> Regards,
> Sasa
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu><mailto:llvm-commits at cs.uiuc.edu><mailto:llvm-commits at cs.uiuc.edu><mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> <prevent-hoisting.patch>
> <prevent-hoisting2.patch>
> <prevent-hoisting3.patch>
> <prevent-hoisting4.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: prevent-hoisting4.patch
Type: text/x-patch
Size: 9257 bytes
Desc: prevent-hoisting4.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140604/4e11731c/attachment.bin>


More information about the llvm-commits mailing list