[PATCH] D47425: [GlobalISel][Legalizer] Fix i1s being sign extended instead of zero-extended

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:756
+    }
+    SrcMO.setCImm(ConstantInt::get(Ctx, WideVal));
 
----------------
dsanders wrote:
> aemerson wrote:
> > rtereshin wrote:
> > > dsanders wrote:
> > > > rtereshin wrote:
> > > > > I don't think this is the right way of achieving this and not the way we seem to be currently handling `G_CONSTANT`s signs across the Global ISel either.
> > > > > 
> > > > > The IR `ConstantInt` that is a part of `G_CONSTANT`'s operand is basically a bag of bits, if it gets extended, the extend is pretty much an anyext, as different usages of it may need different extensions / may interpret the bits differently.
> > > > > 
> > > > > Therefore every use that is not happy with anyext is explicitly extending the value the right way.
> > > > > 
> > > > > Try `./bin/llc -O0 -mtriple aarch64-- div.ll -o - -stop-after=legalizer -simplify-mir` on the following snippets for example:
> > > > > ```
> > > > > define i8 @udiv_with_constants(i8 %a) {
> > > > > entry:
> > > > >   %res = udiv i8 %a, 250
> > > > >   ret i8 %res
> > > > > }
> > > > > ```
> > > > > ```
> > > > > define i8 @sdiv_with_constants(i8 %a) {
> > > > > entry:
> > > > >   %res = sdiv i8 %a, 250
> > > > >   ret i8 %res
> > > > > }
> > > > > ```
> > > > > 
> > > > > There are other instructions that are sensitive to the extensions of constants, like all the shifts, for instance. They solve this problem the same way as divisions / remainders.
> > > > > 
> > > > > Technically, the same constant may have more than one usage that would interpret it differently WRT to sign as well.
> > > > > 
> > > > > So this is ABI boundary problem, not legalizing `G_CONSTANT`s problem, IMO.
> > > > > 
> > > > > I think we need to explicitly extend / mask values before storing them into memory / after loading them from memory, before or after returning them from functions / passing them into functions.
> > > > > 
> > > > > `i1`s are also a little special. The comment to `TargetLowering::getBooleanContents` suggests that it's only meaningful for `i1` produced by a limited set of instructions and "not to be confused with general values promoted from i1", that's probably why some targets prefer to call such instructions producing `i1` legal as soon as those values are immediately used by another instructions from the same subset of special instructions, and then just select the whole thing.
> > > > > 
> > > > > In other words, the flow of `i1`s is limited.
> > > > > 
> > > > > I think for the test case included this needs to be a change to `AArch64CallLowering::lowerReturn` method instead.
> > > > > 
> > > > > + @aditya_nandakumar 
> > > > > + @dsanders 
> > > > There's actually two issues here:
> > > > 1. How do we widen scalars?
> > > > 2. How do we match the ABI?
> > > > 
> > > > For widening scalars, the answer is that we any-extend. However, we can't actually extend with undefined bits so we've been using sext as a placeholder. G_TRUNC/G_SEXT/G_ZEXT can then update the constant as they get optimized. Some targets find it easier to materialize signed immediates and some find unsigned immediates easier so I can see some value in asking the target which it wants to do. That's more of an optimization thing rather than an ABI correctness thing though and it's not really connected with the representation of s1.
> > > > 
> > > > For matching the ABI, targets that implement the signext and zeroext attributes can use them to ensure that LLVM inserts a G_SEXT/G_ZEXT as appropriate to extend the returned result. In principle, I think we should be using that. In practice however, most targets have been getting away with not using those attributes for a very long time (instead, they're just consistent about the extend they use) and we're not going to be a drop-in replacement if we don't behave the same way as before without them. I think the right place to drive this in AArch64CallLowering::lowerReturn(). If that code injects an appropriate extend then the ABI will be correct.
> > > Agreed. I would like to add also, that judging from this https://reviews.llvm.org/D44410#1036451 comment `TargetLowering::getBooleanContents` might not be the right way to ask a target about its preferences even optimization-wise.
> > > 
> > > Now I'm worried about the usage of that method in our `G_STORE` [[ https://github.com/llvm-mirror/llvm/blob/fca3b88d1712d83035c91bc0875233543aab412d/lib/CodeGen/GlobalISel/LegalizerHelper.cpp#L724-L739 | legalization code ]], looks like we should be always zero-extending when storing to memory. + at efriedma
> > We can't look at more than one instruction when we legalise, so won't we need to always zero extend i1s?
> > Now I'm worried about the usage of that method in our G_STORE legalization code, looks like we should be always zero-extending when storing to memory
> 
> That one is directly dealing with ABI issues so it seems correct to me that it's asking the target which extend it wants to use for the store (that doesn't mean that getBooleanContents() is the right function for the query though). clang should be inserting code to convert it to 0 or 1 when it needs the value to be correct w.r.t C/C++.
> 
> > We can't look at more than one instruction when we legalise, so won't we need to always zero extend i1s?
> 
> Could you elaborate on this? I believe we're only looking at one instruction at a time.
Yes never mind I was confused with what we were currently doing with our store legalisation.

I can't find any place where the targets have specified before which kind of extension they want for stores.


Repository:
  rL LLVM

https://reviews.llvm.org/D47425