[PATCH] D93811: [VE] Support llvm.eh.sjlj.lsda

Mon Jan 4 07:02:15 PST 2021

kaz7 added inline comments.

================
Comment at: llvm/lib/Target/VE/VEISelLowering.cpp:1550
+    const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+    MVT PtrVT = TLI.getPointerTy(DAG.getDataLayout());
+    const VETargetMachine *TM =
----------------
simoll wrote:
> kaz7 wrote:
> > simoll wrote:
> > > I suppose you could re-use the type of `Op` here
> > I'm not sure about it.  Do you have any strategies to decide either Op.getValueType() or TLI.getPointerTy()?  All other architectures use TLI.getPointerTy() to implement lsda.  It's really difficult to decide it with certain reasons.
> When isel calls `LowerOperation`, all types are MVTs because all EVTs have been legalized. `Op.getValueType()` should give you the exact same MVT as `TLI.getPointerTy()`.
> AFAIK only `LowerOperationWrapper` is where you have to be careful about operand EVTs but we are not using this in upstream (yet).
I don't understand the point of this.  If you say re-using Op's type is better, I think it may be true.  Then, I watch other architectures using TLI.getPointerTy a lot, I think following others may be better.  Do you know why other architectures use TLI.getPoitnerTy so often?  For historical reason or something?  Anyway, I change it as you suggested.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93811