[LLVMdev] Asking for a quick review

[Tim Northover]

Hi Dmitry,

> I'm not sure that I can explain why it works, but it
> (hopefully) is, so I really need a feedback with this.

I doubt anyone is going to approve the patch without being able to
explain why it works.

To me, it looks a little dodgy. Why should the sign of the offset
depend on its position in the instruction using it? I can vaguely see
there might be some argument that we have to be careful about the 4
variants "B+O", "B-O", "O+B", "O-B" but the code a few lines later
seems to be trying to handle that ("if (SUB && OffsetIdx == 1)").
That's where I'd be inclined to place my change if this bit of code
really is wrong.

At the very least the change is subtle enough that I'd want a clear
explanation in a nearby comment, I think. Hopefully that explanation
can make it clear the patch is correct too.

Also, there may be some misunderstanding about what's needed for a
small test-case. I think what Bill was after is a small .ll test-case
(as seen in test/CodeGen) which gets run whenever anyone runs "make
check" and tests this feature. It would be committed alongside the
change to the actual code.

Cheers.

Tim.