546408 - CodegenLIR code cleanup needed for improving verifier semantics

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P2)

Description

[Edwin Smith]

tracker for cleanup patches needed for bug 413522 (upgrade verifier semantics)

Comment 1

[Edwin Smith]

Attachment: Rename a few 'q' stragglers to 'f' now that nanojit has explicit float64 types

Comment 2

[Edwin Smith]

Attachment: introduce haveDebugger flag, to encapsulate how we test for debugger presence. allows some ifdef removal.

Comment 3

[Edwin Smith]

Attachment: Rename a few 'q' stragglers to 'f' now that nanojit has explicit float64 types (v2)

Comment 4

[Edwin Smith]

Attachment: Rename a few helpers for Number from 'q' -> 'f', properly name writeFixExceptionsAndLabels

Comment 5

[Steven Johnson]

Comment on attachment 427126 [details] [diff] [review]
introduce haveDebugger flag, to encapsulate how we test for debugger presence.  allows some ifdef removal.

Where does haveDebugger get set?

Comment 6

[Edwin Smith]

Attachment: (v2) introduce haveDebugger flag, to encapsulate how we test for debugger presence. allows some ifdef removal.

ah, the perils of extracting small patches from a larger one.  This version initializes haveDebugger.

Comment 7

[Steven Johnson]

Comment on attachment 427154 [details] [diff] [review]
(v2) introduce haveDebugger flag, to encapsulate how we test for debugger presence. allows some ifdef removal.

DEBUGGER_ONLY indentation a little wonky, might recheck before landing

Comment 8

[Edwin Smith]

Attachment: removing some not very useful code

ValidateWriter or Reader should filter illegal opcode usage, the deadvars optimizer isn't the place for it.

local variable vars is redundant and hides CodegenLIR::vars.

lirbuf is dead once vars is undeclared.

Comment 9

[Edwin Smith]

Attachment: verbose changes to help diagnose verify output, and cleanup

* factored verifier.showState() so we can use it to print state without printing an opcode along with it.

* combined & simplified CodegenLIR::formatOpcode 

* fix typo in tagprof macro invocation in initprop_late(): s/ojb/obj/

* print final LIR instructions after deadvars() pass, instead of the input.  (we could print both, but i've never needed to look at the input separate from the mixed verifier/lir output).

* test LC_Liveness before printing live analysis output.

* show merged at verifier block beginnings instead of before the merge, since the merge also explicitly prints the before states and after states.

* fix comments

Comment 10

[Edwin Smith]

Comment on attachment 427182 [details] [diff] [review]
verbose changes to help diagnose verify output, and cleanup

Looking for general feedback, this patch might grow before its ready to land.

Comment 11

[Rick Reitmaier]

Comment on attachment 427182 [details] [diff] [review]
verbose changes to help diagnose verify output, and cleanup

+'d but 2 changes seemed to be out of place for this patch:
(1) deadvars() call
(2) coder->  addition

Comment 12

[Edwin Smith]

Comment on attachment 427182 [details] [diff] [review]
verbose changes to help diagnose verify output, and cleanup

Updated patch to condense the verifier output, and moved to new bug 547240

Comment 13

[Edwin Smith]

Comment on attachment 427131 [details] [diff] [review]
Rename a few helpers for Number from 'q' -> 'f', properly name writeFixExceptionsAndLabels

The q->f renames are leftovers from nanojit supporting floats.

the rename of fixExceptionsAndLabels to writeFixExceptionsAndLabels was an oversight from when the CodeWriter interface was added.  The implementation in CodegenLIR is an empty method, so the rename is benign.

Comment 14

[Edwin Smith]

Comment on attachment 427154 [details] [diff] [review]
(v2) introduce haveDebugger flag, to encapsulate how we test for debugger presence. allows some ifdef removal.

pushed
http://hg.mozilla.org/tamarin-redux/rev/79aa1193fa95

Comment 15

[Edwin Smith]

Comment on attachment 427131 [details] [diff] [review]
Rename a few helpers for Number from 'q' -> 'f', properly name writeFixExceptionsAndLabels

pushed
http://hg.mozilla.org/tamarin-redux/rev/8cfb0efb95b3

Comment 16

[Edwin Smith]

Attachment: (v2) removing some not very useful code in deadvars() passes

Removing local variables that hide global ones with the same name.

Comment 17

[Edwin Smith]

Comment on attachment 428707 [details] [diff] [review]
(v2) removing some not very useful code in deadvars() passes

pushed 
http://hg.mozilla.org/tamarin-redux/rev/8801408fa933

Comment 18

[Edwin Smith]

Attachment: Removes "scope" local variable in Verifier, its used too rarely to be hoisted this way.

Comment 19

[Edwin Smith]

pushed 
http://hg.mozilla.org/tamarin-redux/rev/62574e3cb0c0

Comment 20

[Brent Baker]

Edwin are there specific testcases that should be written to verify this bug or is running the entire acceptance suite and ATS sufficient?

Comment 21

[Edwin Smith]

code cleanup only, no tests needed.