This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Add a pass to fold null checks into nearby memory operations.
ClosedPublic

Authored by sanjoy on Jun 2 2015, 2:40 PM.

Details

Summary

This change adds an "ImplicitNullChecks" target dependent pass. This
pass folds null checks into memory operation using the FAULTING_LOAD
pseudo-op introduced in previous patches.

Depends on D10197
Depends on D10199
Depends on D10200

Diff Detail

Event Timeline

sanjoy updated this revision to Diff 27010.Jun 2 2015, 2:40 PM
sanjoy retitled this revision from to [CodeGen] Add a pass to fold null checks into nearby memory operations..
sanjoy updated this object.
sanjoy edited the test plan for this revision. (Show Details)
sanjoy added reviewers: atrick, reames, rnk, pgavlin.
sanjoy added a subscriber: Unknown Object (MLST).
JosephTremoulet added inline comments.
lib/CodeGen/ImplicitNullChecks.cpp
128–131

Just curious: do you still intend to eventually have this check branch weights and only do the transformation when the null path is very cold? And/or to add some other heuristics here? Or is the plan now to be this aggressive here and rely entirely on patching to handle the cases where the null path is not very cold?

sanjoy added a comment.Jun 5 2015, 9:10 AM

Just curious: do you still intend to eventually have this check branch weights and only do the transformation when the null path is very cold? And/or to add some other heuristics here? Or is the plan now to be this aggressive here and rely entirely on patching to handle the cases where the null path is not very cold?

I plan to do the former. I kept that logic out of this patch to keep
the initial review simple.

  • Sanjoy

http://reviews.llvm.org/D10201

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/
ab added a subscriber: ab.Jun 8 2015, 11:35 AM
ab added inline comments.
include/llvm/CodeGen/Passes.h
555–556

triple slash?

lib/CodeGen/ImplicitNullChecks.cpp
47

"t -> "T

53

Why not class? For that matter, why not private for the non-override members?

73

why not:

NullCheck() : MemOperation(), ...

?

Also, CheckOperation is missing.

116

How about the more common name, "Changed"? Also, this should be updated somewhere. Instead, perhaps simply return !WorkList.empty(), or fold return true into the rewriteNullChecks block?

128–130

Why not triple-slash? (for the other methods as well)

140

This is what I feared with the AnalyzeBranchPredicate patch: I'm not comfortable with mentioning CmpInst here. A couple equally-icky alternatives:

  • 'using CmpInst::Predicate' in MachineBranchPredicate
  • duplicating it for the cases we're interested in, just NE/EQ ?
218–220

Why not chaining BuildMI().addSym().addImm?

223

Except for the first operand, does this ever happen?
If not, can you change the for() to ignore it instead, and turn this into an assert on LoadMI->getOperand(0) ?

254

I don't like this SmallVector<>() construct, but I don't have a better idea ;)

266

Is this actually used?

lib/CodeGen/Passes.cpp
89–92

Can you move this elsewhere, say before PrintLSR? I feel like it doesn't belong here.

sanjoy updated this revision to Diff 27426.Jun 9 2015, 7:42 PM
  • address review
  • make the pass work with updates in dependencies
sanjoy added a comment.Jun 9 2015, 7:43 PM

ping

include/llvm/CodeGen/Passes.h
555–556

Fixed.

lib/CodeGen/ImplicitNullChecks.cpp
47

Fixed.

53

Fixed.

73

Fixed.

116

Fixed.

128–130

Fixed.

140

The second option seems reasonable, I'll go with that.

218–220

Fixed.

223

The first two operands are defs for ADD32rm like instructions.

266

Removed.

lib/CodeGen/Passes.cpp
89–92

Makes sense, this isn't a diagnostic option.

ab added inline comments.Jun 11 2015, 12:44 PM
lib/CodeGen/ImplicitNullChecks.cpp
101–103

Is this necessary?

135

How about 'using TargetInstrInfo::MachineBranchPredicate;'?

193–196

Would (MayLoad && !Predicable) be (conservatively) correct?

214

I'm not sure an assert is appropriate: I don't have an example but this sounds easy to trigger. Should this be checked before even adding the NullCheck to the worklist?

256

With r239553, you should be able to replace this with the IMO much more readable:

/*Cond=*/None, DL);
sanjoy updated this revision to Diff 27547.Jun 11 2015, 2:38 PM
  • address review
lib/CodeGen/ImplicitNullChecks.cpp
101–103

Nope, thanks for catching!

135

That does not seem to compile.

193–196

Should be, unless there are other ways MayLoad instructions can avoid actually doing the load.

214

Kept the assert for documentation, but added a check before adding to worklist.

256

Done.

atrick edited edge metadata.Jun 12 2015, 6:47 PM

Awesome. Nice job splitting the patches.

A few comments inline..

lib/CodeGen/ImplicitNullChecks.cpp
46–49

Huh... There's no platform API for this? It's probably fine since this pass will only be used by people who know what they're doing. I'm just surprised.

202–203

Do we need to introduce a new term "WrappedLoad"? It initially confused me a bit. Why not just stick with FaultingLoad?

218–219

MachineDesc.getNumDefs() does not include any implicit defs that may have been tacked onto the end of the load. However, I think MO.isDef() will still return true for those loads so you'll end up dropping them. Rather than checking for Defs you could just iterate over MachineInst.uses(). Then you'll naturally skip the defs that you want to skip. Yes, it's crazy that MachineOperand.uses() also includes implicit defs.

228–229

I think the name WorkList is misleading, because once you have analyzed all blocks, the list never grows. It's just a plain old list of null check candidates. To make it clear that the list is immutable, rewriteNullChecks can take ArrayRef<NullCheck>.

sanjoy updated this revision to Diff 27630.Jun 12 2015, 9:19 PM
sanjoy edited edge metadata.
  • address review
lib/CodeGen/ImplicitNullChecks.cpp
46–49

There is Process::getPageSize but we need the page size for the target, not the host. For a JIT they'll be the same, but I'd rather not bake that assumption in.

202–203

Fixed.

218–219

Fixed.

228–229

Fixed.

atrick accepted this revision.Jun 12 2015, 9:35 PM
atrick edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 12 2015, 9:35 PM
This revision was automatically updated to reflect the committed changes.