This is the first in a series of patches to make implicit null checks
more general. This patch identifies instructions that do not change
nullness of a register and considers that as a valid instruction to
hoist along with the faulting load. See added testcases.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
| Time | Test | |
|---|---|---|
| 40 ms | linux > Clang.CodeGen::arm-eabi.c | |
| 70 ms | windows > Clang.CodeGen::arm-eabi.c |
Event Timeline
| llvm/test/CodeGen/X86/implicit-null-check.ll | ||
|---|---|---|
| 120 | FYI - all changes below are cosmetic ones from auto generation of check lines with llc. I initially ran this with just the --function option but wasn't sure if it's worth the bother, given it's been a while this test file is updated. | |
| llvm/lib/Target/X86/X86InstrInfo.cpp | ||
|---|---|---|
| 3670 | Nit: Since you check specific opcodes anyway, that check is unnecessary, IMHO | |
| 3677 | Again, since you're checking specific opcodes, you know that opnd 0 and 1 are registers, return MI->getOperand(0).getReg() == NullValueReg && MI->getOperand(1).getReg() == NullValueReg; I.e., no need to check MO.isReg and direct register comparision might be more readable that isTied | |
| llvm/lib/Target/X86/X86InstrInfo.cpp | ||
|---|---|---|
| 3670 | We need this check because if you see the caller in ImplicitNullChecks, it was previously checking if If MI re-defines the PointerReg (i.e. the NullValueReg). We now have both checks within isNullBehaviourUnchanged. I originally thought of having the modifiesRegister in the caller itself and leaving an assert here instead of the check, but I felt this is better. | |
| 3677 | agreed. | |
Comment Actions
Looks good for me.
The only thing that worries me is shouldn't we have broader audience since we're changing 'global' APIs?
| llvm/lib/Target/X86/X86InstrInfo.cpp | ||
|---|---|---|
| 3670 | Yeah, my bad, I've misread the code. :) If it is not modifies register, then it's safe. | |
Comment Actions
addressed review comments around using simpler form rather than isTied. Whitespace testcase changes avoided. NFC w.r.t. prev diff.
| llvm/test/CodeGen/X86/implicit-null-check.ll | ||
|---|---|---|
| 120 | ignore comment. was w.r.t prev diff. all whitespace changes avoided now, | |
Comment Actions
thanks Denis for your comments. I'll let others have a chance in review as well. Regarding broader audience today, implicit null checks works only with X86 (or rather that's what we test with). Also, none of the global APIs are changed, precisely because they are used in other parts of the code. Hence a new API is introduced and used currently only for implicit null checks. It is broad enough that we can reuse it in other passes if we wish to do so.
Comment Actions
Approach generally looks reasonable. Please address nitpicks and then I plan to LGTM.
| llvm/include/llvm/CodeGen/TargetInstrInfo.h | ||
|---|---|---|
| 1276 | minor name change: preservesZeroReg(...) Null is simply zero in MI land, and we don't want to imply there's anything special about the pointer interpretation here. I'd also suggest being really explicit in the comment that the register can become null. This function only returns whether a zero is guaranteed to remain a zero. | |
| llvm/lib/Target/X86/X86InstrInfo.cpp | ||
| 3673 | Minor: Use a switch instead of a series of if clauses please. Also, I believe you can handle all the rr variants here. Also, please assert that operand 0 is a def and operand 1 is a use. Mostly just to make it easier for someone with minimal context to understand form. | |
| llvm/test/CodeGen/X86/implicit-null-check.ll | ||
| 657 | not an LEA - I think you meant complex addressing? Or maybe the LEA gets folded late? Regardless, the comment is out of sync with tested output. | |
| 685 | Please add a couple of counter examples. Particularly: | |
| llvm/lib/Target/X86/X86InstrInfo.cpp | ||
|---|---|---|
| 3673 | I tried writing up the register variant and got compile errors that such opcodes don't exist (i.e. SHL64rr etc..). Checked the intel guide and indeed we only support ri and mi variants, i.e. the MI.getOperand(2) is guaranteed to be an immediate. | |
| llvm/test/CodeGen/X86/implicit-null-check.ll | ||
| 685 | wrong operand order for shift doesn't work (see above comment about rr variant). will add the different instruction case. | |
| llvm/test/CodeGen/X86/implicit-null-check.ll | ||
|---|---|---|
| 685 | I might have to play around with the IR to get the %rax "ri" form working the way we want, i.e. don't convert to implicit null check. Today the "add ri" form doesn't exist before implicit null check. X86 does a pretty decent job and patterns such as "add immediate OR shift, add immediate" is converted into complex addressing mode load. not_null: + %y = ptrtoint i64* %x to i64 + %shry = shl i64 %y, 6 + %shry.add = add i64 %shry, 1 + %y.ptr = inttoptr i64 %shry.add to i64* + %x.loc = getelementptr i64, i64* %y.ptr, i64 1 + %t = load i64, i64* %x.loc + ret i64 %t becomes this before implicit-null-checks: liveins: $rdi renamable $rdi = SHL64ri killed renamable $rdi(tied-def 0), 6, implicit-def dead $eflags renamable $rax = MOV64rm killed renamable $rdi, 1, $noreg, 9, $noreg :: (load 8 from %ir.x.loc) <-- rax = rdi + 1 * 0 + 9 RETQ $rax where rdi is the ZeroValueReg. So, I'm adding an "rr" form and that works as a counter example. | |
minor name change: preservesZeroReg(...)
Null is simply zero in MI land, and we don't want to imply there's anything special about the pointer interpretation here.
I'd also suggest being really explicit in the comment that the register can become null. This function only returns whether a zero is guaranteed to remain a zero.