Page MenuHomePhabricator

[ImplicitNullCheck] Handle instructions that do not modify null behaviour of null checked reg

Authored by anna on Sep 3 2020, 12:07 PM.



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.

Diff Detail

Event Timeline

anna created this revision.Sep 3 2020, 12:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2020, 12:08 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
anna requested review of this revision.Sep 3 2020, 12:08 PM
anna added inline comments.Sep 3 2020, 12:09 PM

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.

dantrushin added inline comments.Sep 3 2020, 12:38 PM

Nit: Since you check specific opcodes anyway, that check is unnecessary, IMHO


Again, since you're checking specific opcodes, you know that opnd 0 and 1 are registers,
so you can do something like

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

anna added inline comments.Sep 4 2020, 3:54 AM

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.



Looks good for me.
The only thing that worries me is shouldn't we have broader audience since we're changing 'global' APIs?


Yeah, my bad, I've misread the code. :) If it is not modifies register, then it's safe.

anna updated this revision to Diff 289925.Sep 4 2020, 5:26 AM

addressed review comments around using simpler form rather than isTied. Whitespace testcase changes avoided. NFC w.r.t. prev diff.

anna added inline comments.Sep 4 2020, 5:28 AM

ignore comment. was w.r.t prev diff. all whitespace changes avoided now,

anna added a comment.Sep 4 2020, 5:31 AM

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.

Approach generally looks reasonable. Please address nitpicks and then I plan to LGTM.


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.


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.


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.


Please add a couple of counter examples. Particularly:
%rax = shift %rdx, %rax (i.e. wrong operand order)
%rax = add64ri %rax, 1 (i.e. a different instruction)

anna added inline comments.Sep 4 2020, 3:24 PM

I'd keep the name as preservesZeroValueInReg because "zero reg" could be confusing to mean "NoRegister" which is 0 for all targets.


ah yes. I meant complex addressing :)

anna added inline comments.Sep 4 2020, 4:09 PM

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.


wrong operand order for shift doesn't work (see above comment about rr variant). will add the different instruction case.

anna added inline comments.Sep 4 2020, 7:23 PM

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.
For example IR such as:

+    %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.

anna updated this revision to Diff 290059.Sep 4 2020, 7:25 PM

addressed review comments.

anna updated this revision to Diff 290559.Sep 8 2020, 11:54 AM

handle clang-tidy comment.

reames accepted this revision.Sep 8 2020, 2:10 PM


This revision is now accepted and ready to land.Sep 8 2020, 2:10 PM
This revision was landed with ongoing or failed builds.Sep 10 2020, 10:40 AM
This revision was automatically updated to reflect the committed changes.