Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27690005:
[MIPS] Modify LongBranch expansion to work with sandboxing (Closed)
|
Created:
7 years, 2 months ago by petarj Modified:
6 years, 9 months ago Reviewers:
Mark Seaborn CC:
native-client-reviews_googlegroups.com Base URL:
http://git.chromium.org/native_client/pnacl-llvm.git@master Visibility:
Public. |
Description[MIPS] Modify LongBranch expansion to work with sandboxing
LongBranch expansion replaces direct branches with indirect jumps in cases
when branch can not reach the target. This assumes the distance from the
target will not change, which is not the case for Native Client and its
sandboxing. For NativeClient, we need to provide flexible LongBranch
expansion that does not rely on the current offset from the target but it
defers offset calculation to the fixup phase after sandboxing is done.
In addition, criteria for LongBranch expansion are more rigid to prevent the
case when the current offset is near but below the threshold yet the
sandboxing pass make it cross the threshold.
BUG= issue found with a large function in llc while compiling pnacl-llc.nexe
TEST= build pnacl-llc.nexe for MIPS
Patch Set 1 #
Total comments: 8
Patch Set 2 : Test case added. #Patch Set 3 : Update. #
Total comments: 20
Patch Set 4 : Changes per code review. #
Total comments: 2
Messages
Total messages: 11 (0 generated)
PTAL.
Can you add an llvm-lit test for this, please? This would be easier to understand with an example. https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... File lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp (right): https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:122: // @LOCALMOD-START This whole function should be inside LOCALMOD comments. https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:128: IsResolved = true; Nit: Put {} brackets around this block since the condition is multi-line https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... File lib/Target/Mips/MCTargetDesc/MipsBaseInfo.h (right): https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... lib/Target/Mips/MCTargetDesc/MipsBaseInfo.h:95: // LOCALMOD-START Nit: The convention is "@LOCALMOD..." (same in other file) https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MipsLongBranc... File lib/Target/Mips/MipsLongBranch.cpp (right): https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MipsLongBranc... lib/Target/Mips/MipsLongBranch.cpp:317: // In PNaCl, modifying sp is not allowed in branch delay slot. Nit: really "NaCl" here; PNaCl does not implement the SFI sandbox
A test case has been added. PTAL. https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... File lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp (right): https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:122: // @LOCALMOD-START On 2013/10/17 19:00:11, Mark Seaborn wrote: > This whole function should be inside LOCALMOD comments. Done. https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:128: IsResolved = true; On 2013/10/17 19:00:11, Mark Seaborn wrote: > Nit: Put {} brackets around this block since the condition is multi-line Done. https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... File lib/Target/Mips/MCTargetDesc/MipsBaseInfo.h (right): https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MCTargetDesc/... lib/Target/Mips/MCTargetDesc/MipsBaseInfo.h:95: // LOCALMOD-START On 2013/10/17 19:00:11, Mark Seaborn wrote: > Nit: The convention is "@LOCALMOD..." (same in other file) Done. https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MipsLongBranc... File lib/Target/Mips/MipsLongBranch.cpp (right): https://codereview.chromium.org/27690005/diff/1/lib/Target/Mips/MipsLongBranc... lib/Target/Mips/MipsLongBranch.cpp:317: // In PNaCl, modifying sp is not allowed in branch delay slot. On 2013/10/17 19:00:11, Mark Seaborn wrote: > Nit: really "NaCl" here; PNaCl does not implement the SFI sandbox Done.
It took me a while to get my head around this... https://codereview.chromium.org/27690005/diff/58001/include/llvm/MC/MCExpr.h File include/llvm/MC/MCExpr.h (right): https://codereview.chromium.org/27690005/diff/58001/include/llvm/MC/MCExpr.h#... include/llvm/MC/MCExpr.h:221: VK_Mips_NACL_LONG_BRANCH_HI16, This produces: native_client/pnacl/git/llvm/lib/MC/MCExpr.cpp:179:10: warning: enumeration value ‘VK_Mips_NACL_LONG_BRANCH_HI16’ not handled in switch [-Wswitch] Maybe you could fix that warning? I think you should also update MipsInstPrinter.cpp, otherwise its asm printer will do 'llvm_unreachable("Invalid kind!")'. Can you add a test for the assembly printing, please? More general design question: GNU 'as' has some support for writing relocations like this: lui $at, %hi(label2 - label1) addiu $at, $at, %lo(label2 - label1) // or lui $at, (label2 - label1) >> 16 addiu $at, $at, (label2 - label1) & 0xffff // (though the latter is not exactly what you want here) label1: ... label2: Is there a reason that wouldn't work for MIPS in LLVM? Does LLVM/MIPS support expressions like that now? If not, would adding support for that be better than adding ad-hoc relocation types? https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MCTargetD... File lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp (right): https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MCTargetD... lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:123: if (OSType == Triple::NaCl) { Isn't this check unnecessary? Checking getKind() below should be enough. https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... File lib/Target/Mips/MipsLongBranch.cpp (right): https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... lib/Target/Mips/MipsLongBranch.cpp:301: // (sanboxing is run after this pass). We therefore replace it with "sandboxing" https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... lib/Target/Mips/MipsLongBranch.cpp:475: // that will be added later. Since at this point we dont know the "don't". Maybe "added later in the MC layer" (which is why it has to come later)? https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... lib/Target/Mips/MipsLongBranch.cpp:476: // exact amount of code that sanboxing will add, we conservatively "sandboxing" https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsMCIns... File lib/Target/Mips/MipsMCInstLower.h (right): https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsMCIns... lib/Target/Mips/MipsMCInstLower.h:37: void LowerLongBranchLUi(const MachineInstr *MI, MCInst &OutMI) const; This isn't defined in this patch. Left over from an earlier version? https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... File test/MC/Mips/nacl-long-branch.ll (right): https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... test/MC/Mips/nacl-long-branch.ll:2: ; RUN: -force-mips-long-branch -O3 -mtriple mipsel-none-nacl %s -o - \ Can you indent these continuation lines for readability? i.e. "RUN: -force-mips..." https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... test/MC/Mips/nacl-long-branch.ll:28: ; 10: bne $4, $zero, 20 Might it be useful to add CHECKs for this case too (via a second RUN above), to check your assumptions? https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... test/MC/Mips/nacl-long-branch.ll:40: ; in the expanded sandboxed long branch sequence the offset that LUi/ADDiu Can you add a comment explaining the arithmetic for this check? Presumably it is: 0x80 - 0x40 = 64 where 0x40 and 0x80 are addresses and 64 is in the "addiu" at 0x40. It took me a while to understand what is going on here. https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... test/MC/Mips/nacl-long-branch.ll:67: ; 5c: jr $1 The instructions from "bal 8" down to here are part of the converted 'long branch' sequence, aren't they? Shouldn't they have CHECKs too (but without checking their hex offsets)?
New patch uploaded. PTAL. Thanks. Petar https://codereview.chromium.org/27690005/diff/58001/include/llvm/MC/MCExpr.h File include/llvm/MC/MCExpr.h (right): https://codereview.chromium.org/27690005/diff/58001/include/llvm/MC/MCExpr.h#... include/llvm/MC/MCExpr.h:221: VK_Mips_NACL_LONG_BRANCH_HI16, On 2013/10/24 23:38:43, Mark Seaborn wrote: > This produces: > native_client/pnacl/git/llvm/lib/MC/MCExpr.cpp:179:10: warning: enumeration > value ‘VK_Mips_NACL_LONG_BRANCH_HI16’ not handled in switch [-Wswitch] > > Maybe you could fix that warning? > > I think you should also update MipsInstPrinter.cpp, otherwise its asm printer > will do 'llvm_unreachable("Invalid kind!")'. Can you add a test for the > assembly printing, please? > > More general design question: > > GNU 'as' has some support for writing relocations like this: > > lui $at, %hi(label2 - label1) > addiu $at, $at, %lo(label2 - label1) > // or > lui $at, (label2 - label1) >> 16 > addiu $at, $at, (label2 - label1) & 0xffff > // (though the latter is not exactly what you want here) > label1: > ... > label2: > > Is there a reason that wouldn't work for MIPS in LLVM? Does LLVM/MIPS support > expressions like that now? If not, would adding support for that be better than > adding ad-hoc relocation types? Done. https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MCTargetD... File lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp (right): https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MCTargetD... lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:123: if (OSType == Triple::NaCl) { On 2013/10/24 23:38:43, Mark Seaborn wrote: > Isn't this check unnecessary? Checking getKind() below should be enough. Done. https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... File lib/Target/Mips/MipsLongBranch.cpp (right): https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... lib/Target/Mips/MipsLongBranch.cpp:301: // (sanboxing is run after this pass). We therefore replace it with On 2013/10/24 23:38:43, Mark Seaborn wrote: > "sandboxing" Done. https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... lib/Target/Mips/MipsLongBranch.cpp:475: // that will be added later. Since at this point we dont know the On 2013/10/24 23:38:43, Mark Seaborn wrote: > "don't". Maybe "added later in the MC layer" (which is why it has to come > later)? Done. https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... lib/Target/Mips/MipsLongBranch.cpp:476: // exact amount of code that sanboxing will add, we conservatively On 2013/10/24 23:38:43, Mark Seaborn wrote: > "sandboxing" Done. https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsMCIns... File lib/Target/Mips/MipsMCInstLower.h (right): https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsMCIns... lib/Target/Mips/MipsMCInstLower.h:37: void LowerLongBranchLUi(const MachineInstr *MI, MCInst &OutMI) const; On 2013/10/24 23:38:43, Mark Seaborn wrote: > This isn't defined in this patch. Left over from an earlier version? Changed to correct function prototypes used in the next version of the patch. https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... File test/MC/Mips/nacl-long-branch.ll (right): https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... test/MC/Mips/nacl-long-branch.ll:2: ; RUN: -force-mips-long-branch -O3 -mtriple mipsel-none-nacl %s -o - \ On 2013/10/24 23:38:43, Mark Seaborn wrote: > Can you indent these continuation lines for readability? > i.e. "RUN: -force-mips..." Done. https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... test/MC/Mips/nacl-long-branch.ll:28: ; 10: bne $4, $zero, 20 On 2013/10/24 23:38:43, Mark Seaborn wrote: > Might it be useful to add CHECKs for this case too (via a second RUN above), to > check your assumptions? Done. https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... test/MC/Mips/nacl-long-branch.ll:40: ; in the expanded sandboxed long branch sequence the offset that LUi/ADDiu On 2013/10/24 23:38:43, Mark Seaborn wrote: > Can you add a comment explaining the arithmetic for this check? > > Presumably it is: > 0x80 - 0x40 = 64 > where 0x40 and 0x80 are addresses and 64 is in the "addiu" at 0x40. > > It took me a while to understand what is going on here. Done. https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... test/MC/Mips/nacl-long-branch.ll:67: ; 5c: jr $1 On 2013/10/24 23:38:43, Mark Seaborn wrote: > The instructions from "bal 8" down to here are part of the converted 'long > branch' sequence, aren't they? Shouldn't they have CHECKs too (but without > checking their hex offsets)? Done.
On 2013/11/21 18:23:42, petarj wrote: > New patch uploaded. PTAL. > > Thanks. > Petar > > https://codereview.chromium.org/27690005/diff/58001/include/llvm/MC/MCExpr.h > File include/llvm/MC/MCExpr.h (right): > > https://codereview.chromium.org/27690005/diff/58001/include/llvm/MC/MCExpr.h#... > include/llvm/MC/MCExpr.h:221: VK_Mips_NACL_LONG_BRANCH_HI16, > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > This produces: > > native_client/pnacl/git/llvm/lib/MC/MCExpr.cpp:179:10: warning: enumeration > > value ‘VK_Mips_NACL_LONG_BRANCH_HI16’ not handled in switch [-Wswitch] > > > > Maybe you could fix that warning? > > > > I think you should also update MipsInstPrinter.cpp, otherwise its asm printer > > will do 'llvm_unreachable("Invalid kind!")'. Can you add a test for the > > assembly printing, please? > > > > More general design question: > > > > GNU 'as' has some support for writing relocations like this: > > > > lui $at, %hi(label2 - label1) > > addiu $at, $at, %lo(label2 - label1) > > // or > > lui $at, (label2 - label1) >> 16 > > addiu $at, $at, (label2 - label1) & 0xffff > > // (though the latter is not exactly what you want here) > > label1: > > ... > > label2: > > > > Is there a reason that wouldn't work for MIPS in LLVM? Does LLVM/MIPS support > > expressions like that now? If not, would adding support for that be better > than > > adding ad-hoc relocation types? > > Done. > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MCTargetD... > File lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp (right): > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MCTargetD... > lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:123: if (OSType == Triple::NaCl) > { > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > Isn't this check unnecessary? Checking getKind() below should be enough. > > Done. > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... > File lib/Target/Mips/MipsLongBranch.cpp (right): > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... > lib/Target/Mips/MipsLongBranch.cpp:301: // (sanboxing is run after this pass). > We therefore replace it with > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > "sandboxing" > > Done. > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... > lib/Target/Mips/MipsLongBranch.cpp:475: // that will be added later. Since at > this point we dont know the > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > "don't". Maybe "added later in the MC layer" (which is why it has to come > > later)? > > Done. > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... > lib/Target/Mips/MipsLongBranch.cpp:476: // exact amount of code that sanboxing > will add, we conservatively > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > "sandboxing" > > Done. > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsMCIns... > File lib/Target/Mips/MipsMCInstLower.h (right): > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsMCIns... > lib/Target/Mips/MipsMCInstLower.h:37: void LowerLongBranchLUi(const MachineInstr > *MI, MCInst &OutMI) const; > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > This isn't defined in this patch. Left over from an earlier version? > > Changed to correct function prototypes used in the next version > of the patch. > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > File test/MC/Mips/nacl-long-branch.ll (right): > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > test/MC/Mips/nacl-long-branch.ll:2: ; RUN: -force-mips-long-branch -O3 -mtriple > mipsel-none-nacl %s -o - \ > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > Can you indent these continuation lines for readability? > > i.e. "RUN: -force-mips..." > > Done. > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > test/MC/Mips/nacl-long-branch.ll:28: ; 10: bne $4, $zero, 20 > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > Might it be useful to add CHECKs for this case too (via a second RUN above), > to > > check your assumptions? > > Done. > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > test/MC/Mips/nacl-long-branch.ll:40: ; in the expanded sandboxed long branch > sequence the offset that LUi/ADDiu > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > Can you add a comment explaining the arithmetic for this check? > > > > Presumably it is: > > 0x80 - 0x40 = 64 > > where 0x40 and 0x80 are addresses and 64 is in the "addiu" at 0x40. > > > > It took me a while to understand what is going on here. > > Done. > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > test/MC/Mips/nacl-long-branch.ll:67: ; 5c: jr $1 > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > The instructions from "bal 8" down to here are part of the converted 'long > > branch' sequence, aren't they? Shouldn't they have CHECKs too (but without > > checking their hex offsets)? > > Done. Ping.
On 2013/11/25 16:57:33, petarj wrote: > On 2013/11/21 18:23:42, petarj wrote: > > New patch uploaded. PTAL. > > > > Thanks. > > Petar > > > > https://codereview.chromium.org/27690005/diff/58001/include/llvm/MC/MCExpr.h > > File include/llvm/MC/MCExpr.h (right): > > > > > https://codereview.chromium.org/27690005/diff/58001/include/llvm/MC/MCExpr.h#... > > include/llvm/MC/MCExpr.h:221: VK_Mips_NACL_LONG_BRANCH_HI16, > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > This produces: > > > native_client/pnacl/git/llvm/lib/MC/MCExpr.cpp:179:10: warning: enumeration > > > value ‘VK_Mips_NACL_LONG_BRANCH_HI16’ not handled in switch [-Wswitch] > > > > > > Maybe you could fix that warning? > > > > > > I think you should also update MipsInstPrinter.cpp, otherwise its asm > printer > > > will do 'llvm_unreachable("Invalid kind!")'. Can you add a test for the > > > assembly printing, please? > > > > > > More general design question: > > > > > > GNU 'as' has some support for writing relocations like this: > > > > > > lui $at, %hi(label2 - label1) > > > addiu $at, $at, %lo(label2 - label1) > > > // or > > > lui $at, (label2 - label1) >> 16 > > > addiu $at, $at, (label2 - label1) & 0xffff > > > // (though the latter is not exactly what you want here) > > > label1: > > > ... > > > label2: > > > > > > Is there a reason that wouldn't work for MIPS in LLVM? Does LLVM/MIPS > support > > > expressions like that now? If not, would adding support for that be better > > than > > > adding ad-hoc relocation types? > > > > Done. > > > > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MCTargetD... > > File lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp (right): > > > > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MCTargetD... > > lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:123: if (OSType == > Triple::NaCl) > > { > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > Isn't this check unnecessary? Checking getKind() below should be enough. > > > > Done. > > > > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... > > File lib/Target/Mips/MipsLongBranch.cpp (right): > > > > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... > > lib/Target/Mips/MipsLongBranch.cpp:301: // (sanboxing is run after this pass). > > We therefore replace it with > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > "sandboxing" > > > > Done. > > > > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... > > lib/Target/Mips/MipsLongBranch.cpp:475: // that will be added later. Since at > > this point we dont know the > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > "don't". Maybe "added later in the MC layer" (which is why it has to come > > > later)? > > > > Done. > > > > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsLongB... > > lib/Target/Mips/MipsLongBranch.cpp:476: // exact amount of code that sanboxing > > will add, we conservatively > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > "sandboxing" > > > > Done. > > > > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsMCIns... > > File lib/Target/Mips/MipsMCInstLower.h (right): > > > > > https://codereview.chromium.org/27690005/diff/58001/lib/Target/Mips/MipsMCIns... > > lib/Target/Mips/MipsMCInstLower.h:37: void LowerLongBranchLUi(const > MachineInstr > > *MI, MCInst &OutMI) const; > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > This isn't defined in this patch. Left over from an earlier version? > > > > Changed to correct function prototypes used in the next version > > of the patch. > > > > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > > File test/MC/Mips/nacl-long-branch.ll (right): > > > > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > > test/MC/Mips/nacl-long-branch.ll:2: ; RUN: -force-mips-long-branch -O3 > -mtriple > > mipsel-none-nacl %s -o - \ > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > Can you indent these continuation lines for readability? > > > i.e. "RUN: -force-mips..." > > > > Done. > > > > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > > test/MC/Mips/nacl-long-branch.ll:28: ; 10: bne $4, $zero, 20 > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > Might it be useful to add CHECKs for this case too (via a second RUN above), > > to > > > check your assumptions? > > > > Done. > > > > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > > test/MC/Mips/nacl-long-branch.ll:40: ; in the expanded sandboxed long branch > > sequence the offset that LUi/ADDiu > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > Can you add a comment explaining the arithmetic for this check? > > > > > > Presumably it is: > > > 0x80 - 0x40 = 64 > > > where 0x40 and 0x80 are addresses and 64 is in the "addiu" at 0x40. > > > > > > It took me a while to understand what is going on here. > > > > Done. > > > > > https://codereview.chromium.org/27690005/diff/58001/test/MC/Mips/nacl-long-br... > > test/MC/Mips/nacl-long-branch.ll:67: ; 5c: jr $1 > > On 2013/10/24 23:38:43, Mark Seaborn wrote: > > > The instructions from "bal 8" down to here are part of the converted 'long > > > branch' sequence, aren't they? Shouldn't they have CHECKs too (but without > > > checking their hex offsets)? > > > > Done. > > Ping. Ping.
https://codereview.chromium.org/27690005/diff/188001/lib/Target/Mips/MCTarget... File lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp (right): https://codereview.chromium.org/27690005/diff/188001/lib/Target/Mips/MCTarget... lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:109: void processFixupValue(const MCAssembler &Asm, const MCAsmLayout &Layout, Can you send this part to upstream LLVM? You can post it to llvm-commits and CC me to review it. This part is generally useful and not NaCl-sandboxing-specific, so I'd like to minimise the number of localmods in our branch. When you do that, can you add an MC-level test for this relocation handling? https://codereview.chromium.org/27690005/diff/188001/lib/Target/Mips/MCTarget... lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:116: // Check for expressions %hi(tmp) and %lo(tmp), where tmp = sym1-sym2. This part isn't MIPS specific, so I wonder if you can reuse some existing arch-independent code for doing this subtraction? MIPS MC can already handle assembly code like this: .4byte sym1 - sym2 which occurs in C++ exception info. Presumably that is handled by an arch-independent part of MC?
On 27 November 2013 19:37, <mseaborn@chromium.org> wrote: > https://codereview.chromium.org/27690005/diff/188001/lib/ > Target/Mips/MCTargetDesc/MipsAsmBackend.cpp > File lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp (right): > > https://codereview.chromium.org/27690005/diff/188001/lib/ > Target/Mips/MCTargetDesc/MipsAsmBackend.cpp#newcode109 > lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:109: void > processFixupValue(const MCAssembler &Asm, const MCAsmLayout &Layout, > Can you send this part to upstream LLVM? You can post it to > llvm-commits and CC me to review it. This part is generally useful and > not NaCl-sandboxing-specific, so I'd like to minimise the number of > localmods in our branch. > > When you do that, can you add an MC-level test for this relocation > handling? Ping. Is my request OK? Am I right in thinking that this relocation handling facility would be generally useful outside of NaCl? Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
On 2013/12/11 21:57:05, Mark Seaborn wrote: > Ping. Is my request OK? Am I right in thinking that this relocation > handling facility would be generally useful outside of NaCl? > You are correct, but before we were able to send that part of the change to the trunk, we need one assembler change first. It got committed on Monday, here it is: https://llvm.org/viewvc/llvm-project?diff_format=c&view=revision&sortby=log&s... so, now we have just been able to send the actual change: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131209/198447.html You have been cc-ed there. Petar
Message was sent while issue was closed.
Since Sasa Stankovic has sent me an upstream patch to implement this now (http://llvm-reviews.chandlerc.com/D3089), I'll close this review. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
