[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:
> craig.topper wrote:
> > 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.
> 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.  
ValVT is always the same as LocVT. The promotion to i64 happens in SelectionDAGBuilder before the setting of ValVT and LocVT happens.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134621