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

[Matt Arsenault via Phabricator via llvm-commits]

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:
> > > > 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.
> > So you have a 32-bit add that uses 64-bit registers, and ignores the high bits of the sources and extends the result? This sounds like how AMDGPU handles 16-bit operations in 32-bit registers. We pass through the 16-bit ops to the selector
> Precisely. I have been trying that as an alternative implementation but without having 32-bit subregisters I don't see how I can create type-legal patterns to select operations operating on 32 bits and create operations which take 32 bit registers. I would do something like `Pat<(i64 (anyext (i32 (OpNode (i32 (trunc GPR:$rs1)), (i32 (trunc GPR:$rs2)))))), (OPW GPR:$rs1, GPR:$rs2)>;, but I definitely cannot guarantee that the anyext/truncates will remain, so I could end up being unable to match the pattern. Without subregisters I can't just select the i32 version of the operation since I'd be replacing it with an i64 version and breaking the type constraints of the instruction selector. For the SelectionDAG code we take the approach that the only legal type is i64 on RV64, then select the 'half-register' variants based on analysing the used bits.
The types added to the register class can be smaller than the full register size. AMDGPU adds 16-bit types to 32-bit registers. The selection process just happens to pick 32-bit register classes in the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76354