| Message ID | 20170807191205.86590-8-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Aug 7, 2017 at 12:12 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > + // replace (A << S) >> S > + // by A & (-1 >> S) Umm. This seems misleading. Don't you mean "value of all ones of the same size as A, shifted right by S" > + insn->src2 = value_pseudo(-1ULL >> value); ..and this seems buggy. There's a lot of bits in -1ULL, but what if the size of 'S' is only 32-bit? So I think you should take the size of A into account? Or am I missing something? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 7, 2017 at 11:54 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Mon, Aug 7, 2017 at 12:12 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: >> + // replace (A << S) >> S >> + // by A & (-1 >> S) > > Umm. This seems misleading. > > Don't you mean "value of all ones of the same size as A, shifted right by S" > >> + insn->src2 = value_pseudo(-1ULL >> value); > > ..and this seems buggy. There's a lot of bits in -1ULL, but what if > the size of 'S' is only 32-bit? > > So I think you should take the size of A into account? > > Or am I missing something? I don't think you're missing something and this code have only been very lightly tested (and maybe developed too fastly too). OTOH, it shouldn't matter much since it's only used for a mask (subsequent steps will drop the superfluous high bits) and I think that it's done so at some other place too. But it certainly wouldn't be bad to use the right size for the mask. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 8, 2017 at 12:08 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Mon, Aug 7, 2017 at 11:54 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> On Mon, Aug 7, 2017 at 12:12 PM, Luc Van Oostenryck >> <luc.vanoostenryck@gmail.com> wrote: >>> + // replace (A << S) >> S >>> + // by A & (-1 >> S) >> >> Umm. This seems misleading. >> >> Don't you mean "value of all ones of the same size as A, shifted right by S" >> >>> + insn->src2 = value_pseudo(-1ULL >> value); >> >> ..and this seems buggy. There's a lot of bits in -1ULL, but what if >> the size of 'S' is only 32-bit? >> >> So I think you should take the size of A into account? >> >> Or am I missing something? You're right. It must be wrong. I think it's working but only because in the tests I've used it's optimized away. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/simplify.c b/simplify.c index f1a898700..cdc244c17 100644 --- a/simplify.c +++ b/simplify.c @@ -416,6 +416,7 @@ static int simplify_lsr(struct instruction *insn, pseudo_t pseudo, long long val { struct instruction *def; unsigned long long mask; + pseudo_t old; if (!value) return replace_with_pseudo(insn, pseudo); @@ -434,6 +435,20 @@ static int simplify_lsr(struct instruction *insn, pseudo_t pseudo, long long val insn->opcode = OP_AND; insn->src2 = value_pseudo(mask >> value); return REPEAT_CSE; + case OP_SHL: + // replace (A << S) >> S + // by A & (-1 >> S) + def = insn->src1->def; + if (!constant(def->src2)) + break; + if (def->src2->value != value) + break; + insn->opcode = OP_AND; + insn->src2 = value_pseudo(-1ULL >> value); + old = insn->src1; + use_pseudo(insn, def->src1, &insn->src1); + remove_usage(old, &insn->src1); + return REPEAT_CSE; } return 0; }
This is especially usefull when simplifying code accessing bitfields. Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- simplify.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)