Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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? |
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/
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:
|
217–219 ↗ | (On Diff #27010) | Why not chaining BuildMI().addSym().addImm? |
222 ↗ | (On Diff #27010) | Except for the first operand, does this ever happen? |
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. |
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. |
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); |
- 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. |
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>. |
- 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. |