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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added a subscriber: aditya_nandakumar.
rtereshin added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:756
+    }
+    SrcMO.setCImm(ConstantInt::get(Ctx, WideVal));
 
----------------
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 


Repository:
  rL LLVM

https://reviews.llvm.org/D47425