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

Nick Desaulniers via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 8 17:30:43 PDT 2019


nickdesaulniers marked 7 inline comments 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:
> nickdesaulniers wrote:
> > 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.
> Ok, I agree that we should be Sign extending, not Z extending.  `SelectionDAG::FoldSymbolOffset` seems to agree (or have the same misunderstanding).  @srhines also points out I dropped the commutative nature of the pre-existing code, so I'll add that back in.
ok, I've switched to SExt from ZExt (as per `SelectionDAG::FoldSymbolOffset`). @t.p.northover can you please triple check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61560





More information about the llvm-commits mailing list