[PATCH] D126110: [BOLT] Fix AND evaluation bug in shrink wrapping

[Amir Ayupov via Phabricator via llvm-commits]

Amir added a comment.

In D126110#3532607 <https://reviews.llvm.org/D126110#3532607>, @rafauler wrote:

> In D126110#3529103 <https://reviews.llvm.org/D126110#3529103>, @Amir wrote:
>
>> I'm concerned that the fix is an overkill and doesn't address the root cause.
>> From what I understand, `evaluateSimple` can only evaluate a constant when given an instruction it understands and a pair of <reg, val> for use in evaluation. So the problem must be with the calling code – that the value of rsp is considered a constant while it's actually not (at least not in this case).
>>
>> Even if it's not the case, I would like to keep the evaluation of `and` – it might be handy in other contexts – but restrict shrink-wrapping from using it.
>
> That makes a lot of sense! Here's what I'm thinking. I would like to have two versions of this function to support this requirement. When we are evaluating offsets (such as stack offsets), only a subset of operations are supported. That's because most of the time we are operating without full knowledge of the actual operands, but rather offsets that are added to an unknown base (x + Offset, and we are doing transformations to Offset to reason about a specific property). In these cases, using other operators that don't have associativity with addition of (x + Offset), where x is the stack base, will break the analysis. In the case of the bug fixed here,  "(x + Offset) AND Constant"  is not the same as  "x + (Offset AND Constant)" (no associativity), hence we can't support AND.  But "(x + Offset) + Displacement+ Index * Size", such as LEA, is the same as "x + (Offset + Displacement + Index * Size)", so it's fine to support LEA as long as we are only feeding Base as the input of the expression. ADD and SUB are supported as well.
>
> But I don't think it's a good idea to add a boolean value  such as "OnlyBaseOffsetAssociativeOperations=true" as a parameter to this function. Rather, I would prefer to break it into two functions, evaluateSimple, used for offsets, and evaluate(), which calls evaluateSimple and if it fails, try a bunch more operations that assume that the operands are fully known during evaluation time. But at the moment, I can't find any places in our codebase that would be users of evaluate() (the more general evaluation function), and thus I'm less inclined to add it as it would be currently dead code. Let me know if you have other ideas on how to move forward. Meanwhile I'll improve the comments on the usage of evaluateSimple to make it clear its intended usage.

Thank you, now I understand the problem and how this change addresses it.
I agree that removing the handling of `AND` and renaming the function to something like `evaluateAssociative` (`simple` is not self-explanatory) would be OK. 
We can add a generic `evaluate` later on when the need arises.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126110/new/

https://reviews.llvm.org/D126110