[PATCH] D92219: [AMDGPU][GlobalISel] Avoid selecting S_PACK with constants

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/Utils.h:146
+                                  bool HandleFConstants = true,
+                                  bool AnyExtAsZext = false);
 const ConstantFP* getConstantFPVRegVal(Register VReg,
----------------
foad wrote:
> paquette wrote:
> > foad wrote:
> > > mbrkusanin wrote:
> > > > foad wrote:
> > > > > I'm not sure if we need a flag for this. Could we do it unconditionally? @qcolombet ?
> > > > There were no regressions for check-all when I tried it that way, but I wasn't sure if I should leave it like that.
> > > I think it's probably safe to treat anyext the same as zext in this function, but maybe wait for comments from other globalisel maintainers (who may be on thanksgiving vacation).
> > Maybe this should be `LookThroughAnyExt`? I imagine there are situations where we might want to treat G_ANYEXT as a sign extend.
> > 
> > e.g.
> > 
> > ```
> > %cst:_(s32) = G_CONSTANT i32 -1
> > %cst_wide:_(s64) = G_ANYEXT %cst
> > ...
> > %y:_(s64) = ...
> > %x:_(s64) = G_ADD %y, %cst_wide
> > ```
> You mean, change the name of the argument to `LookThroughAnyExt`? But what would it do? Currently this function has to either sign- or zero-extend whatever constant it finds, so that it can return a value of the right bit width.
> Currently this function has to either sign- or zero-extend whatever constant it finds

I think Jessica was pointing out that which extend (if any) is the correct thing to do is context and target dependent.

To add to the target dependent side of that: G_ANYEXT is an extension with undefined bits and you're allowed to pick zero/sign-extend if that's convenient for your target but different targets will prefer sign or zero extend and some won't care either way (e.g. because they can operate on the smaller bitwidth). In the case of Mips, it's potentially width dependent due to some quirks of forward compatibility on that target where registers are theoretically sign extended to infinite width even if the value is supposed to be zero extended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92219