[PATCH] D35821: Have ARM call setBooleanContents(ZeroOrOneBooleanContent).

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

There are two patches here, please split them.

It may sound silly to split such small patches, but they're both behaviour changing, and it'd be easier to revert one and not the other if they're separate.

It'd also be clear what tests belong where, and to make sure we have tests for both cases.

cheers,
--renato



================
Comment at: test/CodeGen/ARM/i1.ll:11
+entry:
+  br i1 undef, label %t, label %f
+
----------------
jgalenson wrote:
> efriedma wrote:
> > What are you trying to test here?
> I couldn't easily come up with a simple test for this, so if you have any suggestions I'd be happy to try them instead.  But my goal was to test that the compiler ensures that the i1 is either 0 or 1 (since this commit says it must be one of them).  So my check for that first "mov" is the important part that is not there without my patch.
I'm not sure this is testing what you want and it may depend on the interpretation of undef. In this case, the compiler has selected to always branch to the first block, thus always returning one. Many things there could change, including the order of the blocks, the default value, etc. which would destroy your test.

What would happen if you cast an i8 into an i1? Would your patch create masks to guarantee only the first bit is set?


================
Comment at: test/CodeGen/ARM/i1.ll:3
+
+define i32 @test1(i1 %c, i32 %x) {
+; CHECK-LABEL: test1:
----------------
arguments are never used, why do you need them?


https://reviews.llvm.org/D35821