Page MenuHomePhabricator

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

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

Details

Summary

X86TargetLowering::LowerAsmOperandForConstraint had better support than
TargetLowering::LowerAsmOperandForConstraint for arbitrary depth
getelementpointers for "i", "n", and "s" extended inline assembly
constraints. Hoist its support from the derived class into the base
class.

Link: https://github.com/ClangBuiltLinux/linux/issues/469

Diff Detail

Repository
rL LLVM

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
3512 ↗(On Diff #198296)

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.

3524 ↗(On Diff #198296)

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
3524 ↗(On Diff #198296)

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
3524 ↗(On Diff #198296)

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
3524 ↗(On Diff #198296)

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
3524 ↗(On Diff #198296)

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
3524 ↗(On Diff #198296)

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
3524 ↗(On Diff #198296)

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 ↗(On Diff #198746)

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
3524 ↗(On Diff #198296)

Yep, I think that bit's OK now.

3506 ↗(On Diff #198746)

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 ↗(On Diff #198746)

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.