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

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill 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);
----------------
arsenm wrote:
> 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
I would need some pattern indicating these operations were originally 32 bits to be detectable, because we could then select the appropriate instructions for doing 32 bit operations on 64 bit registers. If I remove the custom legalization here I lose that information because the operation just appears as a 64 bit operation. I could make this work by doing some custom selection code to check if only the lower 32 bits of the result are used. The other alternative is to make these operations legal for 32 bits for these operations (which isn't how the SelectionDAG approaches this issue) and then select the 32 bit operations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76354