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

Repository
rL LLVM

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
127–130 ↗(On Diff #27010)

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
553–554 ↗(On Diff #27010)

triple slash?

lib/CodeGen/ImplicitNullChecks.cpp
46 ↗(On Diff #27010)

"t -> "T

52 ↗(On Diff #27010)

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

72 ↗(On Diff #27010)

why not:

NullCheck() : MemOperation(), ...

?

Also, CheckOperation is missing.

115 ↗(On Diff #27010)

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?

127–129 ↗(On Diff #27010)

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

139 ↗(On Diff #27010)

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 ?
217–219 ↗(On Diff #27010)

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

222 ↗(On Diff #27010)

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) ?

253 ↗(On Diff #27010)

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

265 ↗(On Diff #27010)

Is this actually used?

lib/CodeGen/Passes.cpp
85–88 ↗(On Diff #27010)

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
553–554 ↗(On Diff #27010)

Fixed.

lib/CodeGen/ImplicitNullChecks.cpp
46 ↗(On Diff #27010)

Fixed.

52 ↗(On Diff #27010)

Fixed.

72 ↗(On Diff #27010)

Fixed.

115 ↗(On Diff #27010)

Fixed.

127–129 ↗(On Diff #27010)

Fixed.

139 ↗(On Diff #27010)

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

217–219 ↗(On Diff #27010)

Fixed.

222 ↗(On Diff #27010)

The first two operands are defs for ADD32rm like instructions.

265 ↗(On Diff #27010)

Removed.

lib/CodeGen/Passes.cpp
85–88 ↗(On Diff #27010)

Makes sense, this isn't a diagnostic option.

ab added inline comments.Jun 11 2015, 12:44 PM
lib/CodeGen/ImplicitNullChecks.cpp
100–102 ↗(On Diff #27426)

Is this necessary?

134 ↗(On Diff #27426)

How about 'using TargetInstrInfo::MachineBranchPredicate;'?

192–195 ↗(On Diff #27426)

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

213 ↗(On Diff #27426)

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?

255 ↗(On Diff #27426)

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
100–102 ↗(On Diff #27426)

Nope, thanks for catching!

134 ↗(On Diff #27426)

That does not seem to compile.

192–195 ↗(On Diff #27426)

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

213 ↗(On Diff #27426)

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

255 ↗(On Diff #27426)

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
45–48 ↗(On Diff #27547)

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.

201–202 ↗(On Diff #27547)

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

217–218 ↗(On Diff #27547)

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.

227–228 ↗(On Diff #27547)

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
45–48 ↗(On Diff #27547)

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.

201–202 ↗(On Diff #27547)

Fixed.

217–218 ↗(On Diff #27547)

Fixed.

227–228 ↗(On Diff #27547)

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.