This is an archive of the discontinued LLVM Phabricator instance.

[TargetLowering] Handle multi depth GEPs w/ inline asm constraints
ClosedPublic

Authored by nickdesaulniers on May 4 2019, 11:03 PM.

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2019, 11:03 PM
  • git-clang-format HEAD~
  • add test case for arm32
t.p.northover added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3488

I think this needs to be uint64_t since the arithmetic SDNodes naturally have 2s-complement behaviour. The final value will still be interpreted in a signed manner, but intermediate calculations need to be define when wrapping.

3500

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.

nickdesaulniers marked 2 inline comments as done.May 8 2019, 10:32 AM
nickdesaulniers added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3500

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?

nickdesaulniers marked an inline comment as not done.
t.p.northover added inline comments.May 8 2019, 1:50 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3500

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.

nickdesaulniers added inline comments.May 8 2019, 2:37 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3500

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)?

nickdesaulniers marked an inline comment as done.May 8 2019, 3:31 PM
nickdesaulniers added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3500

Also, it looks like SelectionDAG::FoldConstantArithmetic() could help here.

nickdesaulniers marked an inline comment as not done.May 8 2019, 4:52 PM
nickdesaulniers added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3500

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.

nickdesaulniers marked an inline comment as not done.
  • check both operands for the constant, use SExt
nickdesaulniers marked 7 inline comments as done.May 8 2019, 5:28 PM
nickdesaulniers added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3500

ok, I've switched to SExt from ZExt (as per SelectionDAG::FoldSymbolOffset). @t.p.northover can you please triple check?

nickdesaulniers marked 2 inline comments as done.May 8 2019, 5:40 PM
nickdesaulniers added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3494

Maybe I should initialize C to nullptr and then keep the C ? SDLoc(C) : SDLoc()? I'm not verify familiar with SDLoc, but it seems that I'm potentially messing up debug info? Should the previous code be constructing empty SDLocs (as opposed to SDLoc(GA)?

nickdesaulniers marked an inline comment as not done.May 8 2019, 5:40 PM
t.p.northover added inline comments.May 9 2019, 2:11 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3500

Yep, I think that bit's OK now.

3506

Subtraction is not commutative, so you need to be more careful here.

  • subtraction is not commutative
nickdesaulniers marked 3 inline comments as done.May 9 2019, 9:28 AM
nickdesaulniers added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3506

Good catch, thank you. How's that look? Any thoughts on my SDLoc comment, above?

t.p.northover accepted this revision.May 10 2019, 4:12 AM

I think it looks reasonable now. Thanks for updating it!

This revision is now accepted and ready to land.May 10 2019, 4:12 AM
This revision was automatically updated to reflect the committed changes.
nickdesaulniers marked an inline comment as done.