[PATCH] D134621: [RISCV][WIP] Teach SExtWRemoval to recognize sign extended values that come from arguments.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:10566
 
+  // If input is sign extended from 32 bits, note it for the SExtWRemoval pass.
+  if (In.isOrigArg()) {
----------------
reames wrote:
> 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.  
> 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.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:10568-10572
+    Argument *OrigArg = MF.getFunction().getArg(In.getOrigArgIndex());
+    if (OrigArg->getType()->isIntegerTy()) {
+      unsigned BitWidth = OrigArg->getType()->getIntegerBitWidth();
+      // An input zero extended from i31 can also be considered sign extended.
+      if ((BitWidth <= 32 && In.Flags.isSExt()) ||
----------------
arsenm wrote:
> Don't see why you need to look at the underlying IR here instead of just relying on the argument flags
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134621