[PATCH] D61560: [TargetLowering] Handle multi depth GEPs w/ inline asm constraints

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3524
+                 ConstraintLetter != 's') {
+        Ops.push_back(DAG.getTargetConstant(Offset + C->getSExtValue(),
                                             SDLoc(C), MVT::i64));
----------------
nickdesaulniers wrote:
> t.p.northover wrote:
> > With the above suggestion, I think you'll need to make sure this extension happens properly. You probably do anyway -- the code below for combining constants can do strange things to the high bits if pointer size is less than i64.
> Thanks for the review.  I'm sorry, but I don't fully understand this comment.  Can you please provide more information or clarification?  Are you suggesting maybe returning early if `Offset + C->getSExtValue()` is greater than `INT_MAX`?
All arithmetic is performed at 64-bits, and the final result is provided at that width. But if the original computation is  32-bits, I think bad things might happen. Consider:

    (add (sub X:i32, 1), -1)

I think that leaves `Offset` as 0xfffffffe because of the odd combination of zero-extending the input and multiplying it by -1 for  `ISD::SUB`. Effectively, some -1s are at 32-bits and some are at 64-bits.

So I think you need to do a manual extension of `Offset` from `Op.getValueType().getSizeInBits()` (not looked up the API, but you get the idea) to i64 before creating the constant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61560