[PATCH] D76354: [RISCV][GlobalISel] Legalize types for ALU operations

Mon Mar 14 09:33:50 PDT 2022

arsenm added inline comments.

================
Comment at: llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp:101-104
+bool RISCVLegalizerInfo::legalizeWOpWithSExt(
+    MachineInstr &MI, MachineRegisterInfo &MRI,
+    MachineIRBuilder &MIRBuilder) const {
+  const LLT s64 = LLT::scalar(64);
----------------
lewis-revill wrote:
> arsenm wrote:
> > lewis-revill wrote:
> > > arsenm wrote:
> > > > You shouldn't have this function, you're just repeating totally ordinary promotion the legalizer will handle by default
> > > It doesn't do this by default though. By default the destination gets widened without a `G_SEXT` or a `G_ZEXT`, so any pattern we could look to select later on ends up disappearing. I can use the `widenScalarSrc` as a helper but the `widenScalarDst` employed by the default lowering will give the wrong behaviour.
> > It does if you set your legalization rules to promote these operations. The new result type is the requested promoted type, with a truncate to the original register. The SextInReg here is unnecessary, and also doesn't really do anything since you're truncating right back to the original result type, discarding the extended bits. 
> Sorry I'm still not getting it. How do I set legalization rules to promote these operations? The only similar options I see are `minScalar`/`clampScalar` which will destroy any pattern we could hope to select? Might you be able to explain more?
Remove your customFor. I'm not sure what you mean by "destroy any pattern we could hope to select" - the extensions and the operation are independently legalized and selected operations. If you would like to see G_SEXT_INREG in your selection patterns (something you cannot rely on), you need to mark G_SEXT_INREG legal and the artifact combiner will produce it for you

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76354