Page MenuHomePhabricator

[ImplicitNullCheck] Extend Implicit Null Check scope by using stores
ClosedPublic

Authored by skatkov on Feb 1 2017, 9:42 AM.

Details

Summary

This change allows usage of store instruction for implicit null check.

Memory Aliasing Analisys is not used and change conservatively supposes
that any store and load may access the same memory. As a result
re-ordering of store-store, store-load and load-store is prohibited.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy added a subscriber: atrick.Feb 1 2017, 9:58 AM
sanjoy edited edge metadata.Feb 1 2017, 11:19 PM

If it is not too complicated, I'd rather have a single FAULTING_OP pseudo-instruction, and wrap loads, stores and load-stores into it. We can then later re-use it for divisions as well (implicit division by zero checking).

Also, please update docs/FaultMaps.rst.

lib/Target/X86/X86MCInstLower.cpp
976 ↗(On Diff #86655)

When is StoreDefRegister not X86::NoRegister?

skatkov updated this revision to Diff 86775.Feb 2 2017, 1:08 AM
skatkov edited the summary of this revision. (Show Details)

Please take a look.

sanjoy requested changes to this revision.Feb 6 2017, 1:40 PM
sanjoy added inline comments.
docs/FaultMaps.rst
49 ↗(On Diff #86775)

Please document the two possible values of this FaultKind field.

include/llvm/CodeGen/FaultMaps.h
29 ↗(On Diff #86775)

I found the earlier classification system better, with a third FaultingLoadStore category. Right now we don't have a place to put addq (%rax), %rbx type instructions, which both load and store.

lib/CodeGen/ImplicitNullChecks.cpp
303 ↗(On Diff #86775)

s/no be/not be/

304 ↗(On Diff #86775)

I'd s/usage of memory aliasing analysis/using alias analysis/

308 ↗(On Diff #86775)

How about SeenLoad |= MI.mayLoad()?

310 ↗(On Diff #86775)

It is more idiomatic to say "alias analysis" instead of "Memory Aliasing Analysis".

312 ↗(On Diff #86775)

Please call this something more descriptive than cont.

583 ↗(On Diff #86775)

s/instr/instruction/

lib/Target/X86/X86MCInstLower.cpp
917 ↗(On Diff #86775)

Later on (in a separate change), perhaps we can rewrite this as:

for (auto &MO : make_range(FaultingMI.operands_begin() + OperandsBeginIdx, FaultingMI.operands_end())) {
  // Use MO instead of I
}
This revision now requires changes to proceed.Feb 6 2017, 1:40 PM

Thank you, for review. I will update a change.

include/llvm/CodeGen/FaultMaps.h
29 ↗(On Diff #86775)

I considered that for that kind of instruction we first read and failed on this :) But ok, I will return it back.

lib/CodeGen/ImplicitNullChecks.cpp
308 ↗(On Diff #86775)

Sanjoy, as I understand in my case mayLoad will be einvoked only if SeenLoad == false, in |= form mayLoad will be invoked always and there is no ||= operator.

skatkov updated this revision to Diff 87367.Feb 6 2017, 11:53 PM
skatkov edited edge metadata.
skatkov marked 6 inline comments as done.
skatkov updated this revision to Diff 87368.Feb 6 2017, 11:58 PM
skatkov marked 2 inline comments as done.
sanjoy accepted this revision.Feb 7 2017, 9:51 AM

lgtm, will check this in for you now after running clang-format

(Please ask Chris Lattner for commit access -- http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access :) )

include/llvm/CodeGen/FaultMaps.h
29 ↗(On Diff #86775)

Okay, if all of the load-and-store instructions load before storing then I'm fine changing it back in a later change (but let's not waste another review cycle on it right now).

lib/CodeGen/ImplicitNullChecks.cpp
308 ↗(On Diff #86775)

I'd normally assume that the host compiler would be smart enough to optimize SeenLoad || MI.MayLoad() into SeenLoad | MI.MayLoad(), but your decision is fine by me.

This revision is now accepted and ready to land.Feb 7 2017, 9:51 AM
This revision was automatically updated to reflect the committed changes.

lgtm, will check this in for you now after running clang-format

(Please ask Chris Lattner for commit access -- http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access :) )

Actually I decided that I will do that after my 10-th patch - some kind of anniversary :)
Thank you, Sanjoy for review and assistance in landing my patches.