Float Constant CSE

Float Constant CSE

Load multiple uses of a floating point constant (between two call
instructions or block start/end) into a variable before its first use.
  t1 = b + 1.0
  t2 = c + 1.0
Gets transformed to:
  t0 = 1.0
  t0_1 = t0
  t1 = b + t0_1
  t2 = c + t0_1
Call instructions reset the procedure, but uses the same variable, just
in case it got a register. We are assuming floating point registers are
not calee saved in general. Example, continuing from before:
  result = call <some function>
  t3 = d + 1.0
Gets transformed to:
  result = call <some function>
  t0_2 = t0
  t3 = d + t0_2

BUG= none
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=5bcc6caf4b1890ef678d67fedf905977a2fc8576

[manasijm, 2016-08-02 22:26:58]

manasijm@google.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@chromium.org,
kschimpf@google.com, stichnot@chromium.org

[Jim Stichnoth, 2016-08-03 04:43:02]

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode802
src/IceCfg.cpp:802: // Load multiple uses of a floating point constant (between
two call
Here is the big comment.

It seems to me that the cleanest, simplest approach would be to replace all uses
of a given pooled constant with a new Variable, assign the constant to the
Variable at the appropriate point (such as before its first use), link the
Variable to the constant, and then let local variable splitting do its work.

This would mean modifying Variable::LinkedTo to be an Operand instead of
Variable, and plumbing that through.  For example, getLinkedToStackRoot() is
renamed to getLinkedToMemRoot and has to check for a little bit more than just
whether something is a Variable with actual uses and without a register
assignment.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode811
src/IceCfg.cpp:811: // Call instructions reset the procedure, but uses the same
variable, just
s/uses/use/

Then reflow to 80-col (i.e. "in" should fit at the end of this line).

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode813
src/IceCfg.cpp:813: // not calee saved in general. Example, continuing from
before:
callee

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode819
src/IceCfg.cpp:819: //   t3 = d + 0_2
t0_2

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode820
src/IceCfg.cpp:820: // TODO(manasijm, stichnot): Figure out how to 'link' t0 to
the stack slot of
Pooled constants like 1.0 don't get a stack slot.  They get a global memory
location.  (That doesn't change the fact that t0's LinkedTo field would need to
be an arbitrary Operand that could end up being a Variable or some kind of
MemOperand.)

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode824
src/IceCfg.cpp:824: TimerMarker T(TimerStack::TT_floatConstantCse, this);
TimerMarker _(...);

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode856
src/IceCfg.cpp:856: auto NewVar = makeVariable(Pair.first->getType());
auto *

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode858
src/IceCfg.cpp:858: // TODO: This is not possible yet.
TODO needs owner.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode859
src/IceCfg.cpp:859: auto Assign = InstAssign::create(Node->getCfg(), NewVar,
Pair.first);
auto *

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode864
src/IceCfg.cpp:864: auto NewVar = makeVariable(Pair.first->getType());
auto *

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode866
src/IceCfg.cpp:866: auto Assign =
auto *

[manasijm, 2016-08-03 16:29:33]

Description was changed from

==========
Float Constant CSE

Load multiple uses of a floating point constant (between two call
instructions or block start/end) into a variable before its first use.
  t1 = b + 1.0
  t2 = c + 1.0
Gets transformed to:
  t0 = 1.0
  t0_1 = t0
  t1 = b + t0_1
  t2 = c + t0_1
Call instructions reset the procedure, but uses the same variable, just
in case it got a register. We are assuming floating point registers are
not calee saved in general. Example, continuing from before:
  result = call <some function>
  t3 = d + 1.0
Gets transformed to:
  result = call <some function>
  t0_2 = t0
  t3 = d + 0_2

BUG= none
==========

to

==========
Float Constant CSE

Load multiple uses of a floating point constant (between two call
instructions or block start/end) into a variable before its first use.
  t1 = b + 1.0
  t2 = c + 1.0
Gets transformed to:
  t0 = 1.0
  t0_1 = t0
  t1 = b + t0_1
  t2 = c + t0_1
Call instructions reset the procedure, but uses the same variable, just
in case it got a register. We are assuming floating point registers are
not calee saved in general. Example, continuing from before:
  result = call <some function>
  t3 = d + 1.0
Gets transformed to:
  result = call <some function>
  t0_2 = t0
  t3 = d + t0_2

BUG= none
==========

[manasijm, 2016-08-03 16:32:26]

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode802
src/IceCfg.cpp:802: // Load multiple uses of a floating point constant (between
two call
On 2016/08/03 04:43:02, stichnot wrote:
> Here is the big comment.
> 
> It seems to me that the cleanest, simplest approach would be to replace all
uses
> of a given pooled constant with a new Variable, assign the constant to the
> Variable at the appropriate point (such as before its first use), link the
> Variable to the constant, and then let local variable splitting do its work.
> 
> This would mean modifying Variable::LinkedTo to be an Operand instead of
> Variable, and plumbing that through.  For example, getLinkedToStackRoot() is
> renamed to getLinkedToMemRoot and has to check for a little bit more than just
> whether something is a Variable with actual uses and without a register
> assignment.

Acknowledged.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode811
src/IceCfg.cpp:811: // Call instructions reset the procedure, but uses the same
variable, just
On 2016/08/03 04:43:02, stichnot wrote:
> s/uses/use/
> 
> Then reflow to 80-col (i.e. "in" should fit at the end of this line).

Done.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode813
src/IceCfg.cpp:813: // not calee saved in general. Example, continuing from
before:
On 2016/08/03 04:43:02, stichnot wrote:
> callee

Done.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode819
src/IceCfg.cpp:819: //   t3 = d + 0_2
On 2016/08/03 04:43:02, stichnot wrote:
> t0_2

Done.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode820
src/IceCfg.cpp:820: // TODO(manasijm, stichnot): Figure out how to 'link' t0 to
the stack slot of
On 2016/08/03 04:43:01, stichnot wrote:
> Pooled constants like 1.0 don't get a stack slot.  They get a global memory
> location.  (That doesn't change the fact that t0's LinkedTo field would need
to
> be an arbitrary Operand that could end up being a Variable or some kind of
> MemOperand.)

Acknowledged.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode824
src/IceCfg.cpp:824: TimerMarker T(TimerStack::TT_floatConstantCse, this);
On 2016/08/03 04:43:02, stichnot wrote:
> TimerMarker _(...);

Done.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode856
src/IceCfg.cpp:856: auto NewVar = makeVariable(Pair.first->getType());
On 2016/08/03 04:43:02, stichnot wrote:
> auto *

Done.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode858
src/IceCfg.cpp:858: // TODO: This is not possible yet.
On 2016/08/03 04:43:02, stichnot wrote:
> TODO needs owner.

Done.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode859
src/IceCfg.cpp:859: auto Assign = InstAssign::create(Node->getCfg(), NewVar,
Pair.first);
On 2016/08/03 04:43:02, stichnot wrote:
> auto *

Done.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode864
src/IceCfg.cpp:864: auto NewVar = makeVariable(Pair.first->getType());
On 2016/08/03 04:43:02, stichnot wrote:
> auto *

Done.

https://codereview.chromium.org/2208523002/diff/1/src/IceCfg.cpp#newcode866
src/IceCfg.cpp:866: auto Assign =
On 2016/08/03 04:43:02, stichnot wrote:
> auto *

Done.

[Jim Stichnoth, 2016-08-04 01:45:39]

otherwise lgtm

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode811
src/IceCfg.cpp:811: // Call instructions reset the procedure, but uses the same
variable, just in
s/uses/use/

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode828
src/IceCfg.cpp:828: llvm::ilist_iterator<Inst> Current =
Node->getInsts().begin();
I would just use "auto" here, I do that quite often for iterators.
(and I see you already do it on the next line...)

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode832
src/IceCfg.cpp:832: if (llvm::isa<InstCall>(*Current)) {
Instead of *Current, please use iteratorToInst(Current) .

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode833
src/IceCfg.cpp:833: Current++;
++Current

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode834
src/IceCfg.cpp:834: assert(Current);
What exactly does this assert?  With a quick search, I can't figure out if
iterators have some interesting bool() overload, otherwise this seems
tautological?

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode836
src/IceCfg.cpp:836: while (Current != End && !llvm::isa<InstCall>(*Current)) {
iteratorToInst(Current)

[manasijm, 2016-08-04 21:25:02]

Description was changed from

==========
Float Constant CSE

Load multiple uses of a floating point constant (between two call
instructions or block start/end) into a variable before its first use.
  t1 = b + 1.0
  t2 = c + 1.0
Gets transformed to:
  t0 = 1.0
  t0_1 = t0
  t1 = b + t0_1
  t2 = c + t0_1
Call instructions reset the procedure, but uses the same variable, just
in case it got a register. We are assuming floating point registers are
not calee saved in general. Example, continuing from before:
  result = call <some function>
  t3 = d + 1.0
Gets transformed to:
  result = call <some function>
  t0_2 = t0
  t3 = d + t0_2

BUG= none
==========

to

==========
Float Constant CSE

Load multiple uses of a floating point constant (between two call
instructions or block start/end) into a variable before its first use.
  t1 = b + 1.0
  t2 = c + 1.0
Gets transformed to:
  t0 = 1.0
  t0_1 = t0
  t1 = b + t0_1
  t2 = c + t0_1
Call instructions reset the procedure, but uses the same variable, just
in case it got a register. We are assuming floating point registers are
not calee saved in general. Example, continuing from before:
  result = call <some function>
  t3 = d + 1.0
Gets transformed to:
  result = call <some function>
  t0_2 = t0
  t3 = d + t0_2

BUG= none
R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[manasijm, 2016-08-04 21:25:03]

Committed patchset #3 (id:40001) manually as
5bcc6caf4b1890ef678d67fedf905977a2fc8576 (presubmit successful).

[manasijm, 2016-08-04 21:29:45]

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode811
src/IceCfg.cpp:811: // Call instructions reset the procedure, but uses the same
variable, just in
On 2016/08/04 01:45:39, stichnot wrote:
> s/uses/use/

Done.

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode828
src/IceCfg.cpp:828: llvm::ilist_iterator<Inst> Current =
Node->getInsts().begin();
On 2016/08/04 01:45:39, stichnot wrote:
> I would just use "auto" here, I do that quite often for iterators.
> (and I see you already do it on the next line...)

Done.

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode832
src/IceCfg.cpp:832: if (llvm::isa<InstCall>(*Current)) {
On 2016/08/04 01:45:39, stichnot wrote:
> Instead of *Current, please use iteratorToInst(Current) .

Done.

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode833
src/IceCfg.cpp:833: Current++;
On 2016/08/04 01:45:39, stichnot wrote:
> ++Current

Done.

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode834
src/IceCfg.cpp:834: assert(Current);
On 2016/08/04 01:45:39, stichnot wrote:
> What exactly does this assert?  With a quick search, I can't figure out if
> iterators have some interesting bool() overload, otherwise this seems
> tautological?

Changed to assert(Current != End)
This will be triggered if Nodes start ending with a InstCall.

https://codereview.chromium.org/2208523002/diff/20001/src/IceCfg.cpp#newcode836
src/IceCfg.cpp:836: while (Current != End && !llvm::isa<InstCall>(*Current)) {
On 2016/08/04 01:45:39, stichnot wrote:
> iteratorToInst(Current)

Done.

[manasijm, 2016-08-04 23:42:46]

Description was changed from

==========
Float Constant CSE

Load multiple uses of a floating point constant (between two call
instructions or block start/end) into a variable before its first use.
  t1 = b + 1.0
  t2 = c + 1.0
Gets transformed to:
  t0 = 1.0
  t0_1 = t0
  t1 = b + t0_1
  t2 = c + t0_1
Call instructions reset the procedure, but uses the same variable, just
in case it got a register. We are assuming floating point registers are
not calee saved in general. Example, continuing from before:
  result = call <some function>
  t3 = d + 1.0
Gets transformed to:
  result = call <some function>
  t0_2 = t0
  t3 = d + t0_2

BUG= none
R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

to

==========
Float Constant CSE

Load multiple uses of a floating point constant (between two call
instructions or block start/end) into a variable before its first use.
  t1 = b + 1.0
  t2 = c + 1.0
Gets transformed to:
  t0 = 1.0
  t0_1 = t0
  t1 = b + t0_1
  t2 = c + t0_1
Call instructions reset the procedure, but uses the same variable, just
in case it got a register. We are assuming floating point registers are
not calee saved in general. Example, continuing from before:
  result = call <some function>
  t3 = d + 1.0
Gets transformed to:
  result = call <some function>
  t0_2 = t0
  t3 = d + t0_2

BUG= none
R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[manasijm, 2016-08-04 23:42:46]

manasijm@google.com changed reviewers:
- kschimpf@chromium.org