Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

simp_compose_and_mask improvements #1347

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

tly000
Copy link

@tly000 tly000 commented Jan 9, 2021

this PR simplifies masked ExprCompose objects by removing parts of the expression that are always zero in the result. It also removes the mask if the mask is not needed. I found some cases where this is desirable:

X = {@16[...] 0 16, X[16:32] 16 32} & 0x1000 
--> 
X = {@16[...] 0 16, 0x0 16 32} & 0x1000
IRDst = ({fcom_c0(...) 0 1, fcom_c1(...) 1 2, fcom_c2(...) 2 3, float_stack_ptr 3 6, fcom_c3(...) 6 7, 0x0 7 8} & 0x41)?(loc_1ec5,loc_1ece)
--> 
IRDst = {fcom_c0(...) 0 1, fcom_c3(...) 1 2}?(loc_1ec5,loc_1ece)

if (int2 + 1) & int2 != 0:
return expr
mask_size = int2.bit_length() + 7 // 8
mask_size = (int2.bit_length() + 7) // 8 * 8
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why is there a +7 here
Can you explain?

Copy link
Author

Choose a reason for hiding this comment

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

the mask size is rounded up to the next multiple of 8. i think this is not actually needed, you could just use int2.bit_length() instead, but this may result in more complex expressions because of this line in the loop:

            if arg.is_int() and offset < mask_size < offset+arg.size:
                arg = ExprSlice(arg, 0, mask_size-offset)

this might be the result without rounding up:

{X 0 8, Y 8 16} & 0x7FFF -> {x 0 8, Y & 0x7F 8 15, 0 15 16}

with rounding up:

{X 0 8, Y 8 16} & 0x7FFF -> {X 0 8, Y 8 16} & 0x7FFF

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. The trick is that in Miasm, ExprCompose can have, and have components of different sizes. Not only 8 bit arguments.
So maybe we will have a problem here. I think we can distinguish 2 big cases.

  • The first one, in which this simplification will be very interesting is the case where we make elements "disappear". (the example you gave)
  • The second one, in which we will propagate the mask into the compose, and this propagation will only create multiple new mask for each (or most) sub arguments. In this case, maybe we don't want to propagate the mask

So maybe we can use this metric to do or not, this simplification
So the code may be:

  • do the simplification as you propose
  • once it's done, see if the number of ExprCompose arguments is reduced (compared to the original one).
  • if yes, return this expression
  • if no, return the original expression
    This way, the simplification should not generate a "more complex" expression (in term of number of operations)

I am aware of the fact that it may do some work for nothing, but hey, the 'simplification' problem is hard 😄

What do you think of this proposition?

Copy link
Author

@tly000 tly000 Sep 17, 2021

Choose a reason for hiding this comment

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

i think this is a good idea, but it would be better to check if the resulting ExprCompose is simpler than the old one. i.e. if parts of the expression were replaced by constants. for me the whole reason for this simplification is to get rid of unneeded ExprIds to reduce the dependencies of the expression.

elif mask_size > offset and mask_size < offset+arg.size and arg.is_int():
out.append(ExprSlice(arg, 0, mask_size-offset))
return ExprCompose(*out).zeroExtend(expr.size)
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, maybe here, instead of a break, juste append a ExprInt(0, arg.size)
This way, you generate generate the ExprCompose with the final elements, and don't need to have special cases at the end of the test.
More over, if I don't mess up, you may result with

{XXXX, 0, 8, ExprInt(0, 8), 8, 16, ExprInt(0, 16), 16, 32} 

But this will be simplified by another rule into:

{XXX, 0, 8, ExprInt(0, 24), 8, 32}

Copy link
Author

Choose a reason for hiding this comment

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

i think this is true

@serpilliere
Copy link
Contributor

Hi @tly000
I have totally missed this one.
It's quite interesting, but I have some remarks on the fix

@serpilliere
Copy link
Contributor

I am sorry for the answer delay.

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.

None yet

2 participants