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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers 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:
> > 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.
> > I think that leaves Offset as 0xfffffffe
> 
> Can you please walk me through the arithmetic on that (I might be mixing up my 2's compliment arithmetic, or order of operations)?
Also, it looks like `SelectionDAG::FoldConstantArithmetic()` could help here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61560