Page MenuHomePhabricator

[RISCV] Teach SExtWRemoval to recognize sign extended values that come from arguments.
ClosedPublic

Authored by craig.topper on Sep 25 2022, 8:54 PM.

Details

Summary

This information is not preserved in MIR today. So this patch adds
information to RISCVMachineFunctionInfo when the vreg is created for
the argument.

Diff Detail

Event Timeline

craig.topper created this revision.Sep 25 2022, 8:54 PM
craig.topper requested review of this revision.Sep 25 2022, 8:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 8:54 PM

clang-format

asb added a comment.Sep 27 2022, 8:46 AM

Playing around with this on the GCC torture suite, I'm seeing a number of small codegen improvements, no miscompiles, and no instruction count regressions worth worrying about (pr67037 gets a few more instructions added than are removed, seems to just be due to slightly different BB structure due to different regalloc choices).

This looks reasonable to me, but I'm not knowledgeable enough of the argument lowering to know if this scheme is sound. I'll defer to others on the review.

llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
321

These declarations can be pulled out of the loop.

reames added inline comments.Sep 28 2022, 8:05 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10711

One thought here.

If this logic is sound, then we could use a similar approach to prove known bits for the RegisterSDNode representing the argument location. This would allow SDAG to eliminate some extends, but more importantly, might allow us to do this in a place more likely to expose any unsoundness in a way it gets caught. Particularly, if we can do that in target independent code.

Looking at the calling code, I think this basically translates to putting an AssertSext after the CopyFromReg? Actually, it looks like the caller already does this for us based on the isZExt and isSExt flags.

I suspect you can rewrite this code as if not split and is sext from the flags. Not sure mind you, just suspect.

arsenm added a subscriber: arsenm.Sep 28 2022, 8:19 AM
arsenm added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10713–10717

Don't see why you need to look at the underlying IR here instead of just relying on the argument flags

llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
9

It feels wrong to me that you would need to optimize these after selection but I guess I don't know why you are seeing these

craig.topper added inline comments.Sep 28 2022, 9:24 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10711

One thought here.

If this logic is sound, then we could use a similar approach to prove known bits for the RegisterSDNode representing the argument location. This would allow SDAG to eliminate some extends, but more importantly, might allow us to do this in a place more likely to expose any unsoundness in a way it gets caught. Particularly, if we can do that in target independent code.

Looking at the calling code, I think this basically translates to putting an AssertSext after the CopyFromReg? Actually, it looks like the caller already does this for us based on the isZExt and isSExt flags.

That is correct. We already do exactly that. The changed test in this patch hit the depth limit on computeKnownBits in SelectionDAG or it would have been optimized.

I suspect you can rewrite this code as if not split and is sext from the flags. Not sure mind you, just suspect.

That wouldn't cover the i8 and i16 zext case. And I don't know how the sext in the flags behaves if someone puts it on an i64 that doesn't need to be promoted. clang wouldn't do that, but that's not the only source of IR.

10713–10717

If I don't look at the original IR, then I would do the wrong thing if someone put a signext attribute on an i33 argument. I also wouldn't be able to handle zeroext for i16 and i8.

craig.topper added inline comments.Sep 28 2022, 9:27 AM
llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
9

Some of them are from SelectionDAG's depth limit in simplifyDemandedBits/computeKnownBits/computeNumSignBits. Some of them are because this handles phi loops and SelectionDAG doesn't. Not sure if there are other reasons.

craig.topper added inline comments.Sep 28 2022, 9:28 AM
llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
9

This pass also converts some instructions to their W forms if it would remove a sext.w later. SelectionDAG doesn't do that.

reames added inline comments.Sep 28 2022, 10:10 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10711

On your last point, I don't think you're correct. Here is some pseudo code:

if (!In.isSplit() && LocVT == i64) {
  auto ValVT =  VA.getValVT() 
  if (In.isSExt() && ValVT in {MVT::is32, i16, i8}
    // set sext32 flag
  } else if (In.isZExt() && ValVT in {i16, i8}) {
    //set sext32 flag
  }
}

This doesn't handle odd sized integers as neatly, but do we care?

To be clear, arguing to be pedantic and to understand. I am not objecting to the approach taken.

craig.topper added inline comments.Sep 28 2022, 10:18 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10711

ValVT is always the same as LocVT. The promotion to i64 happens in SelectionDAGBuilder before the setting of ValVT and LocVT happens.

craig.topper added inline comments.Sep 28 2022, 10:28 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10711

AArch64 also has code to recover the original VT from the IR

for (unsigned i = 0; i != NumArgs; ++i) {
  MVT ValVT = Ins[i].VT;
  if (Ins[i].isOrigArg()) {
    std::advance(CurOrigArg, Ins[i].getOrigArgIndex() - CurArgIdx);
    CurArgIdx = Ins[i].getOrigArgIndex();

    // Get type of the original argument.
    EVT ActualVT = getValueType(DAG.getDataLayout(), CurOrigArg->getType(),
                                /*AllowUnknown*/ true);
    MVT ActualMVT = ActualVT.isSimple() ? ActualVT.getSimpleVT() : MVT::Other;
    // If ActualMVT is i1/i8/i16, we should set LocVT to i8/i8/i16.
    if (ActualMVT == MVT::i1 || ActualMVT == MVT::i8)
      ValVT = MVT::i8;
    else if (ActualMVT == MVT::i16)
      ValVT = MVT::i16;
  }
reames added inline comments.Sep 28 2022, 10:35 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10711

Ah, agreed. I'd missed the order of operation happening here.

Add test cases

craig.topper retitled this revision from [RISCV][WIP] Teach SExtWRemoval to recognize sign extended values that come from arguments. to [RISCV] Teach SExtWRemoval to recognize sign extended values that come from arguments..Oct 4 2022, 1:38 PM
craig.topper edited the summary of this revision. (Show Details)
reames accepted this revision.Oct 4 2022, 2:50 PM

LGTM

This revision is now accepted and ready to land.Oct 4 2022, 2:50 PM