Skip to content

Adding Shortcircuit Detection#109

Open
jestrang wants to merge 19 commits intoaayasin:masterfrom
jestrang:shortcircuit
Open

Adding Shortcircuit Detection#109
jestrang wants to merge 19 commits intoaayasin:masterfrom
jestrang:shortcircuit

Conversation

@jestrang
Copy link
Copy Markdown
Contributor

Adding shortcircuit Detection into sampling-LBR.

lbr/lbr.py Outdated
if entry_pt == 0:
candidate_regs=x86.get("srcs",lines[x]) + [x86.get("dst",lines[x])]
for i in candidate_regs:
if '0x' in i:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the purpose of this '0x' check?
if the goal to avoid memory access, then I believe there is a suitable method to leverage from x86.py

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So its filtering out random immediate values as possible candidate registers. For example it will consume "cmp rax,0x1000" and will have [rax,0x1000] in the list, so that removes the 0x1000

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh.. then this is not generic enough; e.g. what if the assembler did not put the immediate value in hexa?

lbr/lbr.py Outdated
elif v2ii2v_dst: inc_stat('V2I transition-Penalty')
if xinfo.is_cond_br() and xinfo.is_taken():
LC.glob['cond_%sward-taken' % ('for' if ip > xip else 'back')] += 1
#Shortcircuit detection
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document in simple language the pattern the heuristic of shortcircuit looks for

Copy link
Copy Markdown
Owner

@aayasin aayasin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

address my 2 comment

@jestrang
Copy link
Copy Markdown
Contributor Author

I've added comments to the shortcircuit branch

Copy link
Copy Markdown
Owner

@aayasin aayasin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

address my 2 comment

if C.any_in(vecs, ''.join(v2ii2v_srcs)): inc_stat('V2I transition-Penalty')
elif C.any_in(vecs, v2ii2v_dst): inc_stat('I2V transition-Penalty')
elif v2ii2v_dst: inc_stat('V2I transition-Penalty')
if size > 1:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems your code needlessly change larger block of code.
For example, this check for size > 1 isn't there after you merge.
Please re-merge your new stats/logic on the latest repo

lbr/lbr.py Outdated
if xinfo.is_cond_br() and xinfo.is_taken():
LC.glob['cond_%sward-taken' % ('for' if ip > xip else 'back')] += 1
#Shortcircuit detection
#A setcc instructions is found, now walk in reverse and find at least two conditional jumps within a max_inst window.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond this 1 lines, Would help the reader to have an example of a sequence of instructions the logic would catch

lbr/lbr.py Outdated
if entry_pt == 0:
candidate_regs=x86.get("srcs",lines[x]) + [x86.get("dst",lines[x])]
for i in candidate_regs:
if '0x' in i:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh.. then this is not generic enough; e.g. what if the assembler did not put the immediate value in hexa?

Copy link
Copy Markdown
Owner

@aayasin aayasin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for documenting the code, Jon.

I had to return it this time since many files under pmu-tools/ change.
Please re-pull changes in a fresh clone area (I merged your other pipeline view).

Use this opportunity to place shortcircuit code in a new function and invoke it from edge_stats(); similar to code in edge_leaf_func_stats(). Breakout long comment line for readability

@jestrang
Copy link
Copy Markdown
Contributor Author

jestrang commented Nov 25, 2024

Will do, I'll put it into a new function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants